Avoid silently overwriting with merge #353
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rwz (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
@rwz would you like me to rebase on master to keep this up to date? |
lib/jbuilder.rb
Outdated
| def _try_merge(current_value, updates) | ||
| if ::Array === updates | ||
| if ::Array === current_value | ||
| return current_value + updates |
There was a problem hiding this comment.
Can we keep the if/else structure and avoid the return keywords?
composerinteralia
force-pushed
the
raise-merge-errors
branch
3 times, most recently
from
9fc0347 to
7e723cf
Compare
At the moment merge! with an array will overwrite any existing hash attributes. It should probably raise a MergeError. It also makes sense to raise this error when attempting to merge a hash into an array, or when trying to merge a non-hash-or-array.
composerinteralia
force-pushed
the
raise-merge-errors
branch
from
7e723cf to
e8ad1e9
Compare
|
@rafaelfranca thanks for your comment! I simplified what a had a bit. This version is closer to the original. |
At the moment
merge!with an array will overwrite any existing hashattributes. It should probably raise a
MergeError. It also makes senseto raise this error when attempting to merge a hash into an array, or
when trying to merge a non-hash-or-array.