Delegate properly to custom Resolver instances #1580
Conversation
|
Example of code that this PR will fix: 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. |
|
cc @JonRowe and @pixeltrix |
|
Needs specs demonstrating what this actually solves vs #1557, in fact ideally we need smoke specs for both "rendering" views and "not rendering" views via this code path. |
|
See my comment #1557 (comment) for why #1557 doesn’t work. I agree that it would be nice to have specs for this (as we discussed in #1551), but they are pretty hard to write. Any suggestions are welcome. |
Take the scenario you posted that supposedly doesn't work and put it into one of the smoke test apps. |
|
I added a smoke spec that passes for this branch, and fails on both master and #1557. Suggestions for cleaning up the spec are much welcome. |
bquorning
force-pushed
the
empty-template-resolver-builder-2
branch
2 times, most recently
from
73ffdd0 to
82de536
Compare
| # Version 3 of mime-types 3 requires Ruby 2.0 | ||
| if RUBY_VERSION < '2.0.0' | ||
| gem 'mime-types', '< 3' | ||
| end |
There was a problem hiding this comment.
https://travis-ci.org/rspec/rspec-rails/builds/118239747 failed to bundle because v2.6.4 of mail is out, which allows bundling with mime-types < 4 instead of, as earlier, mime-types < 3.
bquorning
force-pushed
the
empty-template-resolver-builder-2
branch
4 times, most recently
from
7f05849 to
72a5331
Compare
|
@JonRowe, @pixeltrix This PR has a green build and is ready for review. |
| get :index | ||
|
|
||
| expect(response).to render_template(:baz) | ||
| end |
There was a problem hiding this comment.
Great start, we just need to check that the views are empty (as render_views hasn't been used) and ideally have a second set of tests that checks that the views can be rendered when specified. Appreciate your work to get this towards the line!
|
@JonRowe You were right, there was a bug in the view rendering code (inside How do the added specs look? (I’ll need to squash the commits before the PR is mergeable) |
bquorning
force-pushed
the
empty-template-resolver-builder-2
branch
from
ca741db to
c4e02b8
Compare
Let `EmptyTemplateResolver` creating instances of either `EmptyTemplateResolver::ResolverDecorator` or `EmptyTemplateResolver::FileSystemResolver`, depending on whether the given path is an `ActionView::Resolver` instance or not. The new `EmptyTemplateResolver::ResolverDecorator` class simply delegates all method calls to its given resolver, checks the return values, and changes collections of `ActionView::Template` instances into templates that render with `EmptyTemplateHandler`. The alternative was to implement both `#find_all` and `#find_all_anywhere`. The latter is only present since Rails 4.2.5.1, and the former has different arity in different Rails versions.
bquorning
force-pushed
the
empty-template-resolver-builder-2
branch
from
27174d2 to
c4bc440
Compare
|
|
||
| def method_missing(name, *args, &block) | ||
| result = @resolver.send(name, *args, &block) | ||
| nullify_templates(result) |
There was a problem hiding this comment.
I realize that inspecting return values’ type isn't particularly good design. The alternative is to override #find_all and #find_all_anywhere depending on Rails version – as far as I recall #find_all changed arity from 5 to 6 at some point.
|
@JonRowe We have specs, a green build and squashed commits. Wanna review? |
|
👍 Thanks |
[skip ci]
Let
EmptyTemplateResolvercreate instances of eitherEmptyTemplateResolver::ResolverDecoratororEmptyTemplateResolver::FileSystemResolver, depending on whether the given path argument is anActionView::Resolverinstance or not.The new
EmptyTemplateResolver::ResolverDecoratorclass simply delegates all method calls to its given resolver, except for#find_alland#find_all_anywherewhich it overrides to return templates that render withEmptyTemplateHandler.Supersedes #1568 and hopefully also #1557.