-
-
Notifications
You must be signed in to change notification settings - Fork 410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix anonymous objects #393
Conversation
In the PR description could you please include a reason for these changes? |
lib/i18n/tests/lookup.rb
Outdated
@@ -42,7 +42,7 @@ def setup | |||
end | |||
|
|||
test "lookup: does not modify the options hash" do | |||
options = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to revert this
Updated description. Waiting for tests to finsih |
Do you have any plans to do new release? This one #392 needs more discussion. PS. sometimes forget to do this: thank you for supporting this gem) |
@@ -172,7 +174,7 @@ def translate(*args) | |||
|
|||
# Wrapper for <tt>translate</tt> that adds <tt>:raise => true</tt>. With | |||
# this option, if no translation is found, it will raise <tt>I18n::MissingTranslationData</tt> | |||
def translate!(key, options={}) | |||
def translate!(key, options = EMPTY_HASH) | |||
translate(key, options.merge(:raise => true)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling merge
on a frozen hash will not work:
irb(main):001:0> a = {}.freeze
=> {}
irb(main):002:0> a.merge!(raise: true)
RuntimeError: can't modify frozen Hash
from (irb):2:in `merge!'
from (irb):2
from /Users/ryanbigg/.rubies/ruby-2.4.2/bin/irb:11:in `<main>'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh this means that it is not covered with tests, need to fix this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait a second. Yes .merge!
will raise an error, because you can not mutate frozen object. But this code uses .merge
without exclamation mark
Sequel also uses trick with |
@stereobooster Would you mind rebasing this PR against master? I'd like to get it merged soon rather than leave it hanging around. |
@radar sorry for delay. Rebased. Waiting for CI to pass |
Thank you a lot for coming back to this @stereobooster. Looks like your build is breaking because Rails master now requires Ruby 2.4+, but we were still running builds on Ruby 2.2, 2.3 for Rails master. I've fixed this now on master with 77067d6, and as long as that branch is green, your branch should be too 🤞 |
* master: Ignore Ruby 2.2.8 + 2.3.5 for Rails master
Done! |
Explanation. Trying to remove creation of anonymous objects
^ This gives 147
^ This gives 0
^ This gives 73
^ This gives 0
It would be nice if ruby has magic comment for this case too, like
# frozen_argument_hash: true
instead of thisEMPTY_HASH
hack