Skip to content
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.

Sign up for GitHub

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

user ruby 2.7 'Forward all arguments' syntax: (...) to forward arguments #78

Merged
merged 1 commit into from
May 21, 2024
Merged

user ruby 2.7 'Forward all arguments' syntax: (...) to forward arguments #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/phraseapp-in-context-editor-ruby/adapters/i18n.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
if defined?(I18n)
module I18n
class << self
def translate_with_phraseapp(*args)
PhraseApp::InContextEditor.backend.translate(*args)
def translate_with_phraseapp(...)
PhraseApp::InContextEditor.backend.translate(...)
end
alias_method :translate_without_phraseapp, :translate
alias_method :translate, :translate_with_phraseapp
Expand Down
36 changes: 8 additions & 28 deletions lib/phraseapp-in-context-editor-ruby/backend_service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,49 +9,29 @@ def initialize(args = {})
self
end

def translate(*args)
if to_be_translated_without_phraseapp?(args)
# *Ruby 2.7+ keyword arguments warning*
#
# This method uses keyword arguments.
# There is a breaking change in ruby that produces warning with ruby 2.7 and won't work as expected with ruby 3.0
# The "hash" parameter must be passed as keyword argument.
#
# Good:
# I18n.t(:salutation, :gender => 'w', :name => 'Smith')
# I18n.t(:salutation, **{ :gender => 'w', :name => 'Smith' })
# I18n.t(:salutation, **any_hash)
#
# Bad:
# I18n.t(:salutation, { :gender => 'w', :name => 'Smith' })
# I18n.t(:salutation, any_hash)
#
kw_args = args[1]
if kw_args.present?
I18n.translate_without_phraseapp(args[0], **kw_args)
else
I18n.translate_without_phraseapp(args[0])
end
def translate(...)
if to_be_translated_without_phraseapp?(...)
I18n.translate_without_phraseapp(...)
else
phraseapp_delegate_for(args)
phraseapp_delegate_for(...)
end
end

protected

def to_be_translated_without_phraseapp?(args)
PhraseApp::InContextEditor.disabled? || has_been_forced_to_resolve_with_phraseapp?(args)
def to_be_translated_without_phraseapp?(...)
PhraseApp::InContextEditor.disabled? || has_been_forced_to_resolve_with_phraseapp?(...)
end

def has_been_forced_to_resolve_with_phraseapp?(args)
def has_been_forced_to_resolve_with_phraseapp?(*args)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed method so it behaves the same as befor.

(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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed method so it behaves the same as befor.

key = given_key_from_args(args)
return nil unless present?(key)

Expand Down
32 changes: 15 additions & 17 deletions spec/phraseapp-in-context-editor-ruby/backend_service_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,13 @@
allow(I18n).to receive(:translate_without_phraseapp).with(key_name).and_return(i18n_translation)
end

subject { phraseapp_service.translate(*args) }

context "phrase is enabled" do
before(:each) do
PhraseApp::InContextEditor.enabled = true
end

context "resolve: false given as argument" do
let(:args) { [key_name, resolve: false] }
subject { phraseapp_service.translate(key_name, resolve: false) }

before(:each) do
allow(I18n).to receive(:translate_without_phraseapp).with(key_name, resolve: false).and_return(i18n_translation)
Expand All @@ -34,35 +32,36 @@
end

context "resolve: true given as argument" do
let(:args) { [key_name, resolve: true] }
subject { phraseapp_service.translate(key_name, resolve: true) }

it { is_expected.to be_a String }
it { is_expected.to eql "{{__phrase_foo.bar__}}" }
end

describe "different arguments given" do
context "default array given", vcr: {cassette_name: "fetch list of keys filtered by fallback key names"} do
let(:args) { [:key, {default: [:first_fallback, :second_fallback]}] }
subject { phraseapp_service.translate(:key, default: [:first_fallback, :second_fallback]) }

it { is_expected.to eql "{{__phrase_key__}}" }
end

context "default string given" do
let(:args) { [:key, {default: "first fallback"}] }
subject { phraseapp_service.translate(:key, default: "first fallback") }

it { is_expected.to eql "{{__phrase_key__}}" }
end

context "scope array given" do
subject { phraseapp_service.translate(:key, scope: [:context]) }
let(:context_key_translation) { double }
let(:args) { [:key, {scope: [:context]}] }

it { is_expected.to eql "{{__phrase_context.key__}}" }
end
end
end

context "phrase is disabled" do
let(:args) { [key_name] }
subject { phraseapp_service.translate(key_name) }

before(:each) do
PhraseApp::InContextEditor.enabled = false
Expand All @@ -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) }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If phrase is disabled the params need to be real keys_word args so I changed all places

Copy link
Contributor

@Bilka2 Bilka2 May 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(:ru_translation) { double }

before(:each) do
Expand All @@ -87,27 +86,26 @@
end

context "default array given" do
let(:args) { [:key, {default: [:first_fallback, :second_fallback]}] }
subject { phraseapp_service.translate(:key, default: [:first_fallback, :second_fallback]) }

it { is_expected.to eql "Translation missing. Options considered were:\n- en.key\n- en.first_fallback\n- en.second_fallback" }
end

context "default string given" do
let(:args) { [:key, {default: "first fallback"}] }
subject { phraseapp_service.translate(:key, default: "first fallback") }

it { is_expected.to eql "first fallback" }
end

context "scope array given" do
let(:context_key_translation) { double }
let(:args) { [:key, {scope: [:context]}] }
context "default string given without key" do
subject { phraseapp_service.translate(default: "first fallback") }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That spec is new and was failing


it { is_expected.to eql "Translation missing: en.context.key" }
it { is_expected.to eql "first fallback" }
end

context "scope array given in rails 3 style" do
context "scope array given" do
subject { phraseapp_service.translate(:key, scope: [:context]) }
let(:context_key_translation) { double }
let(:args) { [:key, scope: [:context]] }

it { is_expected.to eql "Translation missing: en.context.key" }
end
Expand Down
Loading