user ruby 2.7 'Forward all arguments' syntax: (...) to forward arguments #78
Conversation
| (args.last.is_a?(Hash) && args.last[:resolve] == false) | ||
| end | ||
|
|
||
| def given_key_from_args(args) | ||
| extract_normalized_key_from_args(args) | ||
| end | ||
|
|
||
| def phraseapp_delegate_for(args) | ||
| def phraseapp_delegate_for(*args) |
There was a problem hiding this comment.
changed method so it behaves the same as befor.
| end | ||
|
|
||
| def has_been_forced_to_resolve_with_phraseapp?(args) | ||
| def has_been_forced_to_resolve_with_phraseapp?(*args) |
There was a problem hiding this comment.
changed method so it behaves the same as befor.
| @@ -71,7 +70,7 @@ | |||
| it { is_expected.to eql i18n_translation } | |||
|
|
|||
| context "given arguments other than key_name" do | |||
| let(:args) { [key_name, locale: :ru] } | |||
| subject { phraseapp_service.translate(key_name, locale: :ru) } | |||
There was a problem hiding this comment.
If phrase is disabled the params need to be real keys_word args so I changed all places
There was a problem hiding this comment.
Note: This is a behaviour change for direct calls to I18n.t when this gem is installed but disabled. Right now on master, I18n.t("foo", {bar: "baz"}) works. With this PR, I18n.t("foo", {bar: "baz"}) errors.
The new behaviour of this PR matches the behaviour of t in views (https://api.rubyonrails.org/classes/ActionView/Helpers/TranslationHelper.html#method-i-translate) both without the gem and with the disabled gem, and I18n.t when this gem is not installed - they all error when a hash is provided instead of keyword arguments.
So I think this is a good change to make.
There was a problem hiding this comment.
Gem disabled
> PhraseApp::InContextEditor.enabled?
=> false
> I18n.t("foo", { bar: "baz" })
=> "Translation missing: de.foo"
Gem not installed
> I18n.t("foo", { bar: "baz" })
.../ruby-3.2.3@mhmr/gems/i18n-1.14.4/lib/i18n.rb:210:in `translate': wrong number of arguments (given 2, expected 0..1) (ArgumentError)
So this could be a breaking change for some apps and should be released in a major version.
Thanks for the review.
| let(:context_key_translation) { double } | ||
| let(:args) { [:key, {scope: [:context]}] } | ||
| context "default string given without key" do | ||
| subject { phraseapp_service.translate(default: "first fallback") } |
There was a problem hiding this comment.
That spec is new and was failing
There was a problem hiding this comment.
I can confirm the issue, otwarchive monkeypatched it. I disabled the linked monkeypatch and then tested this PR in my otwarchive dev environment (Ruby 3.1): I can confirm that this PR fixes the issue.
Varpuspaavi
force-pushed
the
forward_all_arguments
branch
from
f8dcfb4 to
11c4cc4
Compare
|
Thank you for your contribution 🙏 |
If the key is omitted in the translation call and a default is given, the result is wrong and confusing.
In Rails till 7.1 this is the case in the ActionView::Helpers::TranslationHelper#translate call:
rails/rails@58c8a93
The params can be passed arround using the 'Forward all arguments' syntax: (...)
https://rubyreferences.github.io/rubychanges/2.7.html#keyword-argument-related-changes