Delegate properly to custom Resolver instances #1568
Conversation
Introduce `EmptyTemplateResolverFactory` for creating instances of either `EmptyTemplateResolverDecorator` or `EmptyTemplateFileSystemResolver`, depending on whether the given path is an `ActionView::Resolver` instance or not. The new `EmptyTemplateResolverDecorator` class simply delegates all method calls to its given resolver, except for `#find_templates` which it overrides to return templates that render with `EmptyTemplateHandler`.
bquorning
force-pushed
the
empty-template-resolver-builder
branch
from
52a2540 to
f705bda
Compare
|
LGTM, but I'd like another review. |
| # | ||
| # @private | ||
| class EmptyTemplateResolver < ::ActionView::FileSystemResolver | ||
| class EmptyTemplateResolverFactory |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Agree on both the allocation and naming concerns. How does EmptyTemplateResolver.build sound?
There was a problem hiding this comment.
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.
|
There, I pushed some changes:
|
|
@JonRowe LGTM |
|
Why are there no travis results for this branch? |
|
(did not mean to close) |
Perhaps because the merge target is not master branch? |
|
Can you provide an example of the code this is intended to fix? I'm still hesitant around this as I haven't seen a real world scenario that's broken by the existing code... |
|
@JonRowe I created a gist at https://gist.github.com/bquorning/6bf534e4b145b1738bb2. It’s a bit contrived, since I didn’t want to create tables and fetch custom templates from the database. |
|
re: example: |
|
By the way, should I rather make a PR against master, so that we can see some Travis runs? |
|
Added 1 commit that allows Travis to run. Please don’t merge the PR with this commit included. |
|
Build failed because of the new version of Rake that has Removed to support Ruby 1.8.x |
bquorning
force-pushed
the
empty-template-resolver-builder
branch
2 times, most recently
from
194fea0 to
2b34a1b
Compare
bquorning
force-pushed
the
empty-template-resolver-builder
branch
from
2b34a1b to
f8dbe7c
Compare
|
Superseded by #1580. |
Let
EmptyTemplateResolvercreating instances of eitherEmptyTemplateResolver::ResolverDecoratororEmptyTemplateResolver::FileSystemResolver, depending on whether the given path is anActionView::Resolverinstance or not.The new
EmptyTemplateResolver::ResolverDecoratorclass simply delegates all method calls to its given resolver, except for#find_templateswhich it overrides to return templates that render withEmptyTemplateHandler.Please note that the merge target is PR #1557, not master.