Skip to content

Delegate properly to custom Resolver instances #1568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Delegate properly to custom Resolver instances #1568

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f705bda
Delegate properly to custom Resolver instances
bquorning Mar 8, 2016
158af81
Add and update comments
bquorning Mar 8, 2016
790b2cd
Use class method for building resolver
bquorning Mar 8, 2016
9ab092c
Nest private classes under EmptyTemplateResolver
bquorning Mar 8, 2016
f8dbe7c
ALLOW TRAVIS TO RUN. DON'T MERGE.
bquorning Mar 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 42 additions & 10 deletions lib/rspec/rails/view_rendering.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,50 @@ def render_views?
self.class.render_views? || !controller.class.respond_to?(:view_paths)
end

# Delegates find_all to the submitted path set and then returns templates
# with modified source
#
# @private
class EmptyTemplateResolver < ::ActionView::FileSystemResolver
class EmptyTemplateResolverFactory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we build thinks like this we normally make them "instance less" to avoid unnecessary allocations, please refactor to a static instance, or better yet a module method. Can I also suggest a better name is desired? Something like DecoratePath or BuildEmptyTemplateResolver

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree on both the allocation and naming concerns. How does EmptyTemplateResolver.build sound?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we could move EmptyTemplateFileSystemResolver and EmptyTemplateResolverDecorator into this EmptyTemplateResolver module (class), since they’re already supposed to be private and are only used by this factory class.

def initialize(path)
@path = path
end

def initialize(path, pattern=nil)
unless path.is_a?(::ActionView::Resolver)
super
def resolver
if @path.is_a?(::ActionView::Resolver)
EmptyTemplateResolverDecorator.new(@path)
else
EmptyTemplateFileSystemResolver.new(@path)
end
end
end

class EmptyTemplateResolverDecorator
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

# Delegates find_templates to the submitted resolver and then returns templates
# with modified source
#
# @private

def initialize(resolver)
@resolver = resolver
end

def method_missing(name, *args, &block)
@resolver.send(name, *args, &block)
end

private

def find_templates(*args)
templates = @resolver.find_templates(*args)
templates.map do |template|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This map could probably now be delegated to outside as it does the same as below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but I don’t really know where to move it. EmptyTemplateHandler?

::ActionView::Template.new(
"",
template.identifier,
EmptyTemplateHandler,
:virtual_path => template.virtual_path,
:format => template.formats
)
end
end
end

# Delegates find_all to the submitted path set and then returns templates
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be find_templates.

# with modified source
#
# @private
class EmptyTemplateFileSystemResolver < ::ActionView::FileSystemResolver
private

def find_templates(*args)
Expand Down Expand Up @@ -88,13 +120,13 @@ def append_view_path(new_path)
private

def _path_decorator(*paths)
paths.map { |path| EmptyTemplateResolver.new(path) }
paths.map { |path| EmptyTemplateResolverFactory.new(path).resolver }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above about allocations.

end
end

# @private
RESOLVER_CACHE = Hash.new do |hash, path|
hash[path] = EmptyTemplateResolver.new(path)
hash[path] = EmptyTemplateResolverFactory.new(path).resolver
end

included do
Expand Down