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

Jump to bottom

Convert translation key to string as necessary #40773

Merged
merged 1 commit into from Dec 9, 2020
Merged

Convert translation key to string as necessary #40773

merged 1 commit into from Dec 9, 2020

Conversation

jonathanhefner
Copy link
Member

Follow-up to #39989.

I18n.translate converts the initial key (but not default keys) to a string before performing a lookup. For parity, we should do the same.

Fixes d81926f#r44945299.

@jonathanhefner
Convert translation key to string as necessary
2c555c8 
Follow-up to rails#39989.

`I18n.translate` converts the initial key (but not `default` keys) to a
string before performing a lookup.  For parity, we should do the same.
@rails-bot rails-bot bot added the actionview label Dec 9, 2020
@jonathanhefner jonathanhefner added this to the 6.1.0 milestone Dec 9, 2020
@rafaelfranca rafaelfranca merged commit 297a6b9 into rails:master Dec 9, 2020
rafaelfranca added a commit that referenced this pull request Dec 9, 2020
@rafaelfranca
Merge pull request #40773 from jonathanhefner/translate-key-to_s
a2d35fb 
Convert translation key to string as necessary
jonathanhefner referenced this pull request Dec 9, 2020
@jonathanhefner
Improve Action View translate helper
d81926f 
This disentangles the control flow between Action View's `translate` and
I18n's `translate`.  In doing so, it fixes a handful of corner cases,
for which tests have now been added.  It also reduces memory
allocations, and improves speed when using a default:

**Memory**

```ruby
require "benchmark/memory"

Benchmark.memory do |x|
  x.report("warmup") { translate(:"translations.foo"); translate(:"translations.html") }
  x.report("text") { translate(:"translations.foo") }
  x.report("html") { translate(:"translations.html") }
  x.report("text 1 default") { translate(:"translations.missing", default: :"translations.foo") }
  x.report("html 1 default") { translate(:"translations.missing", default: :"translations.html") }
  x.report("text 2 defaults") { translate(:"translations.missing", default: [:"translations.missing", :"translations.foo"]) }
  x.report("html 2 defaults") { translate(:"translations.missing", default: [:"translations.missing", :"translations.html"]) }
end
```

Before:

```
                text     1.240k memsize (     0.000  retained)
                        13.000  objects (     0.000  retained)
                         2.000  strings (     0.000  retained)
                html     1.600k memsize (     0.000  retained)
                        19.000  objects (     0.000  retained)
                         2.000  strings (     0.000  retained)
      text 1 default     4.728k memsize (     1.200k retained)
                        39.000  objects (     4.000  retained)
                         5.000  strings (     0.000  retained)
      html 1 default     5.056k memsize (     1.160k retained)
                        41.000  objects (     3.000  retained)
                         4.000  strings (     0.000  retained)
     text 2 defaults     7.464k memsize (     2.392k retained)
                        54.000  objects (     6.000  retained)
                         4.000  strings (     0.000  retained)
     html 2 defaults     7.944k memsize (     2.384k retained)
                        60.000  objects (     6.000  retained)
                         4.000  strings (     0.000  retained)
```

After:

```
                text   952.000  memsize (     0.000  retained)
                         9.000  objects (     0.000  retained)
                         1.000  strings (     0.000  retained)
                html     1.008k memsize (     0.000  retained)
                        10.000  objects (     0.000  retained)
                         1.000  strings (     0.000  retained)
      text 1 default     2.400k memsize (    40.000  retained)
                        24.000  objects (     1.000  retained)
                         4.000  strings (     0.000  retained)
      html 1 default     2.464k memsize (     0.000  retained)
                        22.000  objects (     0.000  retained)
                         2.000  strings (     0.000  retained)
     text 2 defaults     3.232k memsize (     0.000  retained)
                        30.000  objects (     0.000  retained)
                         2.000  strings (     0.000  retained)
     html 2 defaults     3.456k memsize (     0.000  retained)
                        32.000  objects (     0.000  retained)
                         2.000  strings (     0.000  retained)
```

**Speed**

```ruby
require "benchmark/ips"

Benchmark.ips do |x|
  x.report("text") { translate(:"translations.foo") }
  x.report("html") { translate(:"translations.html") }
  x.report("text 1 default") { translate(:"translations.missing", default: :"translations.foo") }
  x.report("html 1 default") { translate(:"translations.missing", default: :"translations.html") }
  x.report("text 2 defaults") { translate(:"translations.missing", default: [:"translations.missing", :"translations.foo"]) }
  x.report("html 2 defaults") { translate(:"translations.missing", default: [:"translations.missing", :"translations.html"]) }
end
```

Before:

```
                text     35.685k (± 0.7%) i/s -    179.050k in   5.017773s
                html     28.569k (± 3.1%) i/s -    143.871k in   5.040128s
      text 1 default     13.953k (± 2.0%) i/s -     70.737k in   5.071651s
      html 1 default     12.507k (± 0.4%) i/s -     63.546k in   5.080908s
     text 2 defaults      9.103k (± 0.3%) i/s -     46.308k in   5.087323s
     html 2 defaults      8.570k (± 4.3%) i/s -     43.071k in   5.034322s
```

After:

```
                text     36.694k (± 2.0%) i/s -    186.864k in   5.094367s
                html     30.415k (± 0.5%) i/s -    152.900k in   5.027226s
      text 1 default     18.095k (± 2.7%) i/s -     91.086k in   5.036857s
      html 1 default     15.934k (± 1.7%) i/s -     80.223k in   5.036085s
     text 2 defaults     12.179k (± 0.6%) i/s -     61.659k in   5.062910s
     html 2 defaults     11.193k (± 2.1%) i/s -     56.406k in   5.041433s
```
@edariedl
Copy link

edariedl commented Dec 10, 2020 •
edited

hey @rafaelfranca, @jonathanhefner this change breaks the case when translation key is nil. It is changed to empty string and I18n raises I18n::ArgumentError instead of returning "translation missing: cz.no key" or :default if provided.

Expected behaviour should be same as I18n.translate

irb(main):001:0> I18n.t(nil)
=> "translation missing: cz.no key"
irb(main):002:0> I18n.t(nil, default: "default")
=> "default"
irb(main):003:0> I18n.t("")
Traceback (most recent call last):
        1: from (irb):3
I18n::ArgumentError (I18n::ArgumentError)

@jonathanhefner jonathanhefner mentioned this pull request Dec 10, 2020
Merged
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Dec 10, 2020
@jonathanhefner
Handle nil translation key
b8b18bb 
`I18n.translate` returns `nil` when given a `nil` key, *unless* a
`default` is also specified.  If a `default` is specified, the `nil` key
is treated as a missing key.

In Rails 6.0, the `translate` helper always returned `nil` when given a
`nil` key.  After rails#40773, the `translate` helper always raised an
`I18n::ArgumentError` when given a `nil` key.  This commit fixes the
`translate` helper to mirror the `I18n.translate` behavior when given a
`nil` key, with and without a `default`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
actionview
Projects
None yet
Milestone
6.1.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jonathanhefner @edariedl @rafaelfranca