Skip to content

support translation hashes with numeric keys in Simple backend#422

Merged
radar merged 1 commit intoruby-i18n:masterfrom
wjordan:simple_numeric_keys
Oct 14, 2018
Merged

support translation hashes with numeric keys in Simple backend#422
radar merged 1 commit intoruby-i18n:masterfrom
wjordan:simple_numeric_keys

Conversation

@wjordan
Copy link
Copy Markdown
Contributor

@wjordan wjordan commented Jul 17, 2018

Currently, loading a translation hash with a Numeric key (e.g., store_translations(:en, 1 => 'foo'); I18n.t(1)) does not work using the Simple backend, since the implementation's call to deep_symbolize_keys (to normalize all translation keys as Symbols) does not affect Numeric types (Numeric#to_sym is not implemented). This PR additionally calls deep_stringify_keys to help normalize key types implementing to_s.

This also brings the behavior of KeyValue and Simple backends slightly closer together, since numeric keys are already supported by the KeyValue backend.

I don't expect much of a performance hit from the added deep_stringify_keys operation. Surprisingly in tests with my use-case, I saw load times with this PR change actually improve by 25% (from 60 seconds to 45 seconds), possibly since the call to deep_symbolize_keys was no longer hitting the NoMethodError exception/ rescue nil code path for Numeric keys in my translation data.

@radar
Copy link
Copy Markdown
Collaborator

radar commented Jul 18, 2018

Hi @wjordan. Thank you for the pull request. Could you please give some more context around how you're using this in your application? Most often, people will use numbers and pair them up with some "context". A price, a label... something else:

I18n.t(:price, amount: 10) => $10 / £10
I18n.t(:unread_messages, count: 2) => 2 unread messages  / 2 mensajes no leídos

Could you show me the context of how you're using this?

@wjordan
Copy link
Copy Markdown
Contributor Author

wjordan commented Jul 18, 2018

Could you please give some more context around how you're using this in your application?

Sure- in my case, numeric keys are being used in a YAML i18n source file for localizing collections of 'slides' in our application, which are indexed by slide number. See slides.en.yml:

en:
  slides:
    maze_intro:
      1:
        image: 'notes/hoc1_1.jpg'
        text: "Welcome to your first hour of code! Let's dive right into your first taste of programming."
      2:
        image: 'notes/hoc1_2.jpg'
        text: "We'll be using Blockly, a visual programming language where you drag and drop blocks to write code."
      3:
        image: 'notes/hoc1_3.jpg'
        text: "Most code is typed, but Blockly is visual. Each blocks corresponds to a line of \"real\" code."
[...]

Without this PR, using the Simple backend I18n.t('slides.maze_intro.1.text') results in a missing translation, the workaround I18n.t('slides.maze_intro')[1][:text] is needed in order to lookup the localized string.

We've considered converting our source YAML to explicitly cast all integer keys as strings ("1": ), but I thought applying this change upstream might be more concise and helpful to others.

This is different from the use of interpolation or pluralization features to inject numbers into the localized-string value, as in your examples.

@radar
Copy link
Copy Markdown
Collaborator

radar commented Dec 17, 2018

Heads up: This PR caused a regression -- see #443.

It will be good to see if we can mitigate this regression by making both the new / old behaviours work. If we cannot, I will be reverting this PR.

radar added a commit that referenced this pull request Dec 17, 2018
This will ensure compatibility with old I18n (1.1.0) (see also issue #443) and what is now expected as of 1.2.0 (see #422)

I have switched this Hash class method monkey patching to use refinements, because it will keep the method overriding contained to the select places of the I18n gem where it is used.
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.

2 participants