672031 - [compare-locales] Warn when the plural form count doesn't match the expected count for the locale

Status: RESOLVED FIXED

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

Description

[Florian Quèze [:florian]]

New translators sometimes don't understand well how the localized plurals work and it's frequent to see them put only 2 plural forms for a locale that needs more.

And for experimented translators, forgetting a plural form is probably a common inattention mistake.

Comment 1

[Axel Hecht [:Pike]]

... funny how pluralRule in intl.properties is also checked by the plurals checker ... and generates warnings and errors, depending on  locale.

Comment 2

[Axel Hecht [:Pike]]

flod, what's your take on error vs warning?

Lack of plural variants could just be a warning, as we're using the first?
Should we make excess variants a warning, or an error?

Comment 3

[Francesco Lodolo [:flod]]

Ideally:
* Excess of variants: warning. You're not breaking anything, you have a form that doesn't get used. In most cases there's an extra semicolon at the end, or it's a locale with 1 form following English, both easy to fix.
* Lack of variants: error. You're generating errors in console, and likely have some poor translations.

There are two concerns in considering lack of variants an error:
* devtools: if locales with more than 2 plural forms copy over English text, they'll get errors.
* we still have a ton of them (257, they were in the 400 a few weeks back), and it might hurt us.

So, we might have to go with warning also for lack of variants.

Comment 4

[Axel Hecht [:Pike]]

Attachment: bug 672031, pass locale to Checker,

Preparing to add plurals checker, we pass the locale from the
l10n file to the checker.

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

Comment 5

[Axel Hecht [:Pike]]

Attachment: bug 672031, factor plurals check in properties to distinct method,

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

Comment 6

[Axel Hecht [:Pike]]

Attachment: bug 672031, add check for plural forms,

Use the list of forms in plurals.py to check if there's an entry
for each of them.
Make those warnings now, as they're likely bugs, but not hard
failures.

The actual `pluralForm` string in intl.properties is also
triggering the plural form logic, so exclude that. Given that we
don't have the file name in the checker, just ignore strings
with that id and a number as value.

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

Comment 7

[Francesco Lodolo [:flod]]

Comment on attachment 8936361 [details]
bug 672031, pass locale to Checker,

https://reviewboard.mozilla.org/r/207078/#review213076

Comment 8

[Francesco Lodolo [:flod]]

Comment on attachment 8936362 [details]
bug 672031, factor plurals check in properties to distinct method,

https://reviewboard.mozilla.org/r/207080/#review213078

Comment 9

[Francesco Lodolo [:flod]]

Comment on attachment 8936363 [details]
bug 672031, add check for plural forms,

https://reviewboard.mozilla.org/r/207082/#review213072

I feel like we should raise a warning if the locale is not available in plurals, not sure where though (definitely not for each string we parse)

::: compare_locales/checks.py:113
(Diff revision 1)
> +            found_forms = l10nValue.count(';') + 1
> +            msg = 'expecting {} plurals, found {}'.format(
> +                expected_forms,
> +                found_forms
> +            )
> +            if expected_forms > found_forms:

This should be only one if

if expected_forms != found_forms:
    yield ('warning', 0, msg, 'plural')

Comment 10

[Axel Hecht [:Pike]]

Comment on attachment 8936363 [details]
bug 672031, add check for plural forms,

https://reviewboard.mozilla.org/r/207082/#review213072

Will add an integration test suite to be run via tox and in automation.

As it depends on an external service, I'd rather not do that as part of the regular tests, though.

> This should be only one if
> 
> if expected_forms != found_forms:
>     yield ('warning', 0, msg, 'plural')

I left it like this so that we can more easily switch either case from warning to error if we decide to do so.

Comment 11

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #10)
> I left it like this so that we can more easily switch either case from
> warning to error if we decide to do so.

Ah, that makes more sense now.

Comment 12

[Axel Hecht [:Pike]]

Attachment: bug 672031, add integration test for plurals

Not part of the usual test cases as it might break for
transvision changes, or changes to locales.
But good to run in automation either way, via tox and travis.

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

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 8936361 [details]
bug 672031, pass locale to Checker,

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

Comment 14

[Axel Hecht [:Pike]]

Comment on attachment 8936362 [details]
bug 672031, factor plurals check in properties to distinct method,

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

Comment 15

[Axel Hecht [:Pike]]

Comment on attachment 8936363 [details]
bug 672031, add check for plural forms,

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

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 8936363 [details]
bug 672031, add check for plural forms,

https://reviewboard.mozilla.org/r/207082/#review213072

Good call, the plural form for Occitan was out of sync :-).
https://hg.mozilla.org/l10n/compare-locales/rev/f2a6a6f2cb6f295b19d438c8e890198630511d06 landed the fix, rebased on top of that.

Comment 17

[Axel Hecht [:Pike]]

https://hg.mozilla.org/l10n/compare-locales/pushloghtml?changeset=caffef49b18c, FIXED.