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.

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

Pass options as keyword arguments to translation procs #529

Merged
merged 1 commit into from
Jun 4, 2020

Conversation

casperisfine
Copy link

So a pattern that's heavily used in our translation data, is to use keyword arguments in translations procs rather than an option hash, e.g:

lambda do |_key, number:, **|
  if number == 1
    'er'
  else
    'e'
end

Instead of:

lambda do |_key, options|
  if options.fetch(:number) == 1
    'er'
  else
    'e'
end

It makes for slightly more concise code, and gives better error messages if a key is missing.

However since the signature I18n call is call(String, Hash) this causes warnings on 2.7 and will break on 2.8/3.0.

gem/ruby/2.7.1/gems/i18n-1.8.2/lib/i18n/backend/base.rb:149: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
config/locales/rules/de.rb:8: warning: The called method `call' is defined here

I can off course rewrite these callbacks, but since I believe using keyword args is often nicer, I wanted to check if there was interest in updating the signature.

AFAIK this is backward compatible, as call(String, **Hash) will automatically pass the keyword args as a positional hash if the proc doesn't accept keyword args:

l = lambda do |_key, options|
  if options.fetch(:number) == 1
    'er'
  else
    'e'
  end
end
hash = { number: 1}
p l.call('foo', **hash) # No warnings

@radar
Copy link
Collaborator

radar commented Jun 4, 2020

Changes look good to me. Tests are happy with them too. Thank you for taking the time to submit this PR.

@radar radar merged commit c0850ea into ruby-i18n:master Jun 4, 2020
@casperisfine
Copy link
Author

Wow, thanks a lot for the quick merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants