Let decorator_class infer anonymous class' decorator from superclass #820
Conversation
|
@masolin Thanks for the contribution! I think this change makes sense, but give me a little bit to think through any unintended side affects. Also, if we do move forward with this, I think it would make sense to update all the other usages of @steveklabnik @jcasimir This change makes sense to me, but wanted to double check with one of you guys in case there is some historical knowledge I'm missing. |
|
It's been too long; I don't remember any details here, sorry! |
|
@codebycliff Please let me know if you think that it's ok to use |
|
@masolin Thanks for your patience on this. I'm good with this change. If you can get the other uses of |
To prevent rescuing and checking if NameError is from internal decorator private methods, we change to return nil on these private methods.
|
Hi @codebycliff Please help to review it. Thanks. |
lib/draper/decorator.rb
Outdated
| name_constant = name && name.safe_constantize | ||
| raise Draper::UninferrableObjectError.new(self) if name_constant.nil? | ||
|
|
||
| name_constant |
There was a problem hiding this comment.
In Decoratable#decorator_class above, you are returning the constant unless it's nil and raise at the end. Here you are doing the exact opposite. For consistency purposes, I think it would make sense to follow the logic from above here as well:
name_constant = name && name.safe_constantize
return name_constant unless name_constant.nil?
raise Draper::UninferrableObjectError.new(self)
lib/draper/decorator.rb
Outdated
| singular = object_class_name | ||
| plural = singular && singular.pluralize | ||
|
|
||
| plural == singular ? nil : "#{plural}Decorator" |
There was a problem hiding this comment.
While the ternary statement works just fine, returning nil is the default behavior. So it might read better to say:
singular = object_class_name
plural = singular && singular.pluralize
"#{plural}Decorator" unless plural == singularWhat do you think?
There was a problem hiding this comment.
I feel that the ternary statement can make me more easily realize the 2 possible return types: nil and "#{plural}Decorator", but I don't have strong opinion on it, so it's fine to change
"#{plural}Decorator" unless plural == singularThere was a problem hiding this comment.
My opinion isn't strong either. I see the advantages of the ternary as well. Completely up to you.
based on code review comments
|
Thanks a lot. 😄 |
|
This has been released to RubyGems. |
Description
Since Rails 5, draper gem gets some issues of raising
NameErroron inferring decorator class (#773). The previous PR (#795) fixes this issue when usingdecorate_collection, but this issue can also happen when usingdecorate, and we encountered this issue on our production code when upgrading to Rails 5.I tried to replace
constantizewithsafe_constantizein this PR, so we don't need to rescue and check if theNameErrorwas caused by draper itself anymore. It should be safer than previous approach.Thanks.
References