1432163 - Update compare-locales CLDR plural mappings

Status: RESOLVED FIXED

(Localization Infrastructure and Tools :: compare-locales, enhancement)

Description

[Francesco Lodolo [:flod]]

A couple of nits and a question

https://hg.mozilla.org/l10n/compare-locales/file/2f8dd5380978/compare_locales/plurals.py#l22
hr: correct order is 'one', 'few', 'other'

https://hg.mozilla.org/l10n/compare-locales/file/2f8dd5380978/compare_locales/plurals.py#l38
ar: 'zero' should be the first in the list

Rule #7 is defined as ('one', 'few', 'many'): that works for ru and uk
http://www.unicode.org/cldr/charts/dev/supplemental/language_plural_rules.html#ru
http://www.unicode.org/cldr/charts/dev/supplemental/language_plural_rules.html#uk

but not for hr and sr, which are defined as ('one', 'few', 'other') in CLDR

http://www.unicode.org/cldr/charts/dev/supplemental/language_plural_rules.html#hr
http://www.unicode.org/cldr/charts/dev/supplemental/language_plural_rules.html#sr

How should we deal with this?

Comment 1

[Axel Hecht [:Pike]]

The order in which we list the keys in compare-locales corresponds to the numerical values in pluralforms.jsm.

n==0 yields 5 there for Arabic, which is why 'zero' is the last in the list.

We'll need to handle hr and sr in a way that allows us to map ints from PluralForms.jsm to words in cldr speak.

Comment 2

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #1)
> The order in which we list the keys in compare-locales corresponds to the
> numerical values in pluralforms.jsm.

Ah, completely missed that :-\

Comment 3

[Axel Hecht [:Pike]]

So, this leaves ru/uk vs bs, hr, sr? I'm tying those three together, as they're one pluralform in cldr.

Should we figure out what comes out of bug 1432151? Or tie those three locales into a single conversation?

What's the deal with other/many in Russian? That rule is sooo hard to parse.

Comment 4

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #3)
> So, this leaves ru/uk vs bs, hr, sr? I'm tying those three together, as
> they're one pluralform in cldr.

Yes

> Should we figure out what comes out of bug 1432151? Or tie those three
> locales into a single conversation?

Agreed

> What's the deal with other/many in Russian? That rule is sooo hard to parse.

I assume 'other' is only used for numbers with decimals. Sadly, the examples associated to those rules are always really confusing (why 1.5 of all the numbers? Look for example at German)

Comment 5

[Francesco Lodolo [:flod]]

http://unicode.org/reports/tr35/tr35-numbers.html#Operands

one, few, many apply to integer with 0 visible decimals (v)
https://github.com/unicode-cldr/cldr-core/blob/master/supplemental/plurals.json#L613-L615

'other' applies only to decimals.

Comment 6

[Francesco Lodolo [:flod]]

Using this bug for a side question, since it's generically about updating the list of mappings:
* I'm currently running the migration on all repositories we have, not just the ones shipping. That includes very old Mercurial repos, and very new BitBucket ones.
* https://github.com/projectfluent/python-fluent/pull/52 moves to use compare-locales instead of CLDR data for plurals migration

Should we add to compare-locales all locales for which we have repositories? I think that would help with migration, but clearly it would require further changes to tests (e.g. skip a locale if not available in official Mercurial repos).

Comment 7

[Francesco Lodolo [:flod]]

Attachment: test.patch

Some experimenting with compare-locales.

This still leaves out kok and pbb, for which I don't have any info (no plural defined in CLDR or in intl.properties)

Comment 8

[Francesco Lodolo [:flod]]

Attachment: testv2.patch

Stricter mapping

Comment 9

[Axel Hecht [:Pike]]

I'm not fond of what this does to the integration test for the plural forms. The way I have hacked that is OK for one locale, but for dozens, not so much.

Makes me wonder about a couple of things. Should these locales be supported on Transvision? Maybe behind a flag?

In general, I think we should convert as many repos as we can.

Comment 10

[Axel Hecht [:Pike]]

Attachment: bug 1432163, add all locales to plurals.py, hack for bs, hr, sr,

Review commit: https://reviewboard.mozilla.org/r/221288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221288/

Comment 11

[Axel Hecht [:Pike]]

Comment on attachment 8952064 [details]
bug 1432163, add all locales to plurals.py, hack for bs, hr, sr,

https://reviewboard.mozilla.org/r/221288/#review227124

::: compare_locales/plurals.py:166
(Diff revision 1)
>      'wo': CATEGORIES_BY_INDEX[0],
>      'xh': CATEGORIES_BY_INDEX[1],
>      'zam': CATEGORIES_BY_INDEX[1],
>      'zh-CN': CATEGORIES_BY_INDEX[1],
> -    'zh-TW': CATEGORIES_BY_INDEX[0]
> +    'zh-TW': CATEGORIES_BY_INDEX[0],
> +    # Non building locales

I'd prefer to keep these inline with the regular locales, so that we don't need to change this file when we add or remove locales.

Comment 12

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #11)
> I'd prefer to keep these inline with the regular locales, so that we don't
> need to change this file when we add or remove locales.

Agreed. I left them at the bottom because I wasn't sure if the patch was usable, and this way it was evident which locales were added.

> Makes me wonder about a couple of things. Should these locales be supported
> on Transvision? Maybe behind a flag?

I'd prefer not, for a few reasons:
* We would need to support 2 categories of locales, one in BitBucket and one in Mozilla. That's likely not much work, but still non trivial.
* Some of these locales are dead, we only have the repos with legacy content. I want to migrate them, in case there's something usable, but nobody will ever use Transvision for them.

Comment 13

[Staś Małolepszy :stas]

Comment on attachment 8952064 [details]
bug 1432163, add all locales to plurals.py, hack for bs, hr, sr,

https://reviewboard.mozilla.org/r/221288/#review227386

::: compare_locales/plurals.py:7
(Diff revision 1)
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  'Mapping of locales to CLDR plural categories as implemented by PluralForm.jsm'
>  
> -CATEGORIES_BY_INDEX = (
> +CATEGORIES_BY_INDEX = {

The only export of this file is CATEGORIES_BY_LOCALE. Should we rename CATEGORIES_BY_INDEX to `_categories_by_index` to indicate its private nature and to hide if from `import *`?

::: compare_locales/plurals.py:71
(Diff revision 1)
>      'be': CATEGORIES_BY_INDEX[7],
>      'bg': CATEGORIES_BY_INDEX[1],
>      'bn-BD': CATEGORIES_BY_INDEX[2],
>      'bn-IN': CATEGORIES_BY_INDEX[2],
>      'br': CATEGORIES_BY_INDEX[1],
> -    'bs': CATEGORIES_BY_INDEX[7],
> +    'bs': CATEGORIES_BY_INDEX[100],

Instead of turning CATEGORIES_BY_INDEX into a dict with the special `100` key, how about we create a new dict: 

    CATEGORIES_EXCEPTIONS = {
        7: ('one', 'few', 'other'),
    }
    
And then in CATEGORIES_BY_LOCALE:

    'bs': CATEGORIES_EXCEPTIONS[7],

Comment 14

[Francesco Lodolo [:flod]]

Attachment: Bug 1432163 - Add locales to plurals, add exceptions for rule 7 and bs, hr, sr

Review commit: https://reviewboard.mozilla.org/r/221594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221594/

Comment 15

[Francesco Lodolo [:flod]]

Comment on attachment 8952342 [details]
Bug 1432163 - Add locales to plurals, add exceptions for rule 7 and bs, hr, sr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221594/diff/1-2/

Comment 16

[Francesco Lodolo [:flod]]

Comment on attachment 8952342 [details]
Bug 1432163 - Add locales to plurals, add exceptions for rule 7 and bs, hr, sr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221594/diff/2-3/

Comment 17

[Francesco Lodolo [:flod]]

Comment on attachment 8952342 [details]
Bug 1432163 - Add locales to plurals, add exceptions for rule 7 and bs, hr, sr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221594/diff/3-4/

Comment 18

[Axel Hecht [:Pike]]

Comment on attachment 8952342 [details]
Bug 1432163 - Add locales to plurals, add exceptions for rule 7 and bs, hr, sr

https://reviewboard.mozilla.org/r/221594/#review228300

I'd like to make the integration tests a bit less verbose, but that's OK for a follow-up.

Comment 19

[Axel Hecht [:Pike]]

https://hg.mozilla.org/l10n/compare-locales/rev/a020d82e928270455d105c7a57588c92d53c8aeb, FIXED.

Thanks for the patch.