Raise ArgumentError if :renderable object does not respond to #render_in
#50665
Conversation
seanpdoyle
force-pushed
the
action-view-renderable-argument-error
branch
from
09f0420
to
3a6c573
Compare
| @@ -5,6 +5,10 @@ class Template | |||
| # = Action View Renderable Template for objects that respond to #render_in | |||
| class Renderable # :nodoc: | |||
| def initialize(renderable) | |||
| unless renderable.respond_to?(:render_in) | |||
| raise ArgumentError, "'#{renderable.inspect}' is not a renderable object. It must implement #render_in." | |||
There was a problem hiding this comment.
I think its unclear from the PR description what problem this is trying to solve.
If the goal is just to raise a friendlier error than NoMethdError in render, then I think we should rescue that when it happens and raise a friendlier message there instead of adding a (slow) respond_to? check to the happy path of Renderable.
There was a problem hiding this comment.
I was about to say that. We don't slow down happy path to help developers to not make self-inflected mistakes. If you know about the : renderable object I'm supposing you know that object needs to respond to render_in. I'm ok with raising a better message when they don't, but not on the happy path.
There was a problem hiding this comment.
I've moved this out of the class and back to TemplateRenderer. It mimics similar ArgumentError blocks for other options like :file
seanpdoyle
force-pushed
the
action-view-renderable-argument-error
branch
from
3a6c573
to
896bc8a
Compare
| @@ -41,7 +41,12 @@ def determine_template(options) | |||
| end | |||
| Template::Inline.new(options[:inline], "inline template", handler, locals: keys, format: format) | |||
| elsif options.key?(:renderable) | |||
| Template::Renderable.new(options[:renderable]) | |||
| renderable = options[:renderable] | |||
| unless renderable.respond_to?(:render_in) | |||
There was a problem hiding this comment.
This still adds overhead in the happy path
There was a problem hiding this comment.
I've changed this to rescue from NoMethodError. Is that what you had in mind?
…ender_in` When calling `render` with a `:renderable` argument, ensure that the object responds to `#render_in`. If it doesn't, raise an `ArgumentError`. This commit also adjusts the `ArgumentError` that when a `:partial` argument isn't Active Model compatible. Prior to this commit, the message used `:` as a prefix to `to_partial_path`. This commit replaces that with a `#` prefix to denote that it's expected to be an instance method on the object.
seanpdoyle
force-pushed
the
action-view-renderable-argument-error
branch
from
896bc8a
to
b3bb06a
Compare
Follow-up to rails#50665. Unconditionally converting `NoMethodError` to `ArgumentError` can mask a legitimate `NoMethodError` from within the `render_in` method. This commit adds a check to prevent that.
Follow-up to rails#50665. Unconditionally converting `NoMethodError` to `ArgumentError` can mask a legitimate `NoMethodError` from within the `render_in` method. This commit adds a check to prevent that.
Detail
When calling
renderwith a:renderableargument, ensure that the object responds to#render_in. If it doesn't, raise anArgumentError.Additional Information
This commit also adjusts the
ArgumentErrorthat when a:partialargument isn't Active Model compatible. Prior to this commit, the message used:as a prefix toto_partial_path. This commit replaces that with a#prefix to denote that it's expected to be an instance method on the object.Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]