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

Add support for CLDR data in I18n::Backend::Pluralization #630

Merged

Conversation

movermeyer
Copy link
Contributor

@movermeyer movermeyer commented Jun 9, 2022

What are you trying to accomplish?

Fixes #629

What approach did you choose and why?

What should reviewers focus on?

This is a breaking change. Anyone using the zero key in locales that don't require the zero key will have to change the key to use the explicit "0" key instead.

Q&A

Q: Are 0 & 1 the only special cases, or can one introduce others? Perhaps 2: I have a pair of cars
A: It might be allowed, depending on how you read the spec... but the spec only mentions 0 and 1, the LDML DTD only considers 0 and 1 to be valid, and CLDR has only ever used 0 and 1. I left support for arbitrary numbers out of this PR, as I figure that it would be better to be restrictive with what we accept right now. It's an easy change to accept other values if it turns out to be desirable.

The impact of these changes

Pluralization lookups will will support CLDR's data.
The problematic use of the zero key will be resolved.
Fixes #629

@movermeyer movermeyer marked this pull request as draft June 9, 2022 14:32
Copy link

@trishrempel trishrempel left a comment

Choose a reason for hiding this comment

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

👍🏻

This is a breaking change. Anyone using the `zero` key in locales that don't require the `zero` key will have to change the key to use the explicit "0" key.
@movermeyer movermeyer force-pushed the movermeyer/pluralization_cldr_support branch from ed22cbd to a7b92a1 Compare June 9, 2022 14:39
@movermeyer movermeyer marked this pull request as ready for review June 9, 2022 14:40
@radar
Copy link
Collaborator

radar commented Jul 10, 2022

Spec mentions:

0 | 1 | zero | one | two | few | many | other

Which, in my experience, covers languages such as Russian that have particular words for "few" and "many" vs two of a thing (iirc)

PR looks good. Thank you very much :)

@radar radar merged commit 9540cf1 into ruby-i18n:master Jul 10, 2022
@simi
Copy link

simi commented Jul 13, 2022

Hello!

I just faced problem when upgrading rubygems.org (https://github.com/rubygems/rubygems.org/runs/7312344176?check_suite_focus=true) since there was different text in kaminari pagination (No gems found before and Displaying all 0 gems after this change).

I was able to track down this to https://github.com/kaminari/kaminari/blob/bf20fccfdbf7d10c337aeead7ecf4f54a230a5fb/kaminari-core/config/locales/kaminari.yml#L19 and fix it (at least temporarily) in app itself by providing following in en.yml locale file.

  helpers:
    page_entries_info:
      one_page:
        display_entries:
          "0": "No %{entry_name} found"

Does it mean providing "zero" as a key for special key value was removed starting 1.11.0 (for languages having zero not part of pluralization config like en https://github.com/svenfuchs/rails-i18n/blob/f5b8ac6707243084ed5357ea4c35ab4b977235c2/lib/rails_i18n/common_pluralizations/one_other.rb#L14)?

If so, that seems like a "drastic" change actually since it is even recommended in Rails Guides and per my experience this kind of unexpected change is hard to spot by automated tests (usually I don't test those edge cases for each pluralized test). Those locales are often shipped with 3rd party gems (like kaminari in my case) as well making it totally invisible to debug.

All applications I have around right now needs to lock dependency before latest release since I would need to scan them manually to ensure all pluralizations using this zero key (custom locales, and gems locales) are handled properly in every language supported. 😨

Is there any chance to reconsider this and make it at least deprecated first, put under configuration flag and change the behaviour later with major release?

@movermeyer
Copy link
Contributor Author

Does it mean providing "zero" as a key for special key value was removed starting 1.11.0 (for languages having zero not part of pluralization config like en

@simi Yes. IMO, this change should be highlighted in the CHANGELOG / release notes.

it is even recommended in Rails Guides

Thanks for the reminder. I've opened a PR to correct the docs.

@sarahraqueld
Copy link

IMO, this change should be highlighted in the CHANGELOG / release notes.

+1

@radar
Copy link
Collaborator

radar commented Jul 13, 2022

Apologies for the trouble everyone. I'll remove this with #633. There should absolutely be no breaking changes between i18n minor versions.

@radar
Copy link
Collaborator

radar commented Jul 13, 2022

I18n v1.12.0 has now been released with this rolled back.

@simi
Copy link

simi commented Jul 14, 2022

Thanks @radar!

Would it be welcomed to re-introduce this in small steps? I can try to prepare new version with deprecation warning for example. But I'm still afraid the impact of this change is enormous.

@radar
Copy link
Collaborator

radar commented Jul 14, 2022

@simi sure, go ahead. Can you figure out how to do the deprecation warning for 1.x, and then we’ll release this change again as a 2.x?

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.

[BUG] I18n::Backend::Pluralization should support CLDR pluralization
5 participants