1473577 - Support running compare-locales against reference

Status: RESOLVED FIXED

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

Description

[Axel Hecht [:Pike]]

Right now, running compare-locales against the reference is a bit tricky.

Let's formally support that.

Comment 1

[Axel Hecht [:Pike]]

Attachment: bug 1473577, add validation of reference support,

We're using None as magic value for validating the reference,
until we're getting to the actual comparison.
Then we're using en-x-moz-reference, which isn't _as_ proof
as None, but is handled better in the result code.

Also adding a plural form for en-x-moz-reference so that
plural checks are run on the reference with English plural
forms.

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

Comment 2

[Staś Małolepszy :stas]

Comment on attachment 8991897 [details]
bug 1473577, add validation of reference support,

https://reviewboard.mozilla.org/r/256810/#review265704

LGTM. I'm not a fan of en-x-moz-reference, however. The fact that it needs to hardcode both "en" and "moz" in there is an indication that perhaps there's a better way to go about this. What if --validate wasn't a flag but instead took a single locale code as its value? This code could then be used to run the plural forms checks.

::: compare_locales/compare.py:668
(Diff revision 1)
>          ):
>      locales = set()
>      observers = []
>      for project in project_configs:
> +        # disable filter if we're in validation mode
> +        if None not in project.locales:

Nit: a positive test (if None in...) would read better here given that it corresponds to the validation special case.

::: compare_locales/paths.py:301
(Diff revision 1)
>          self.mergebase = mergebase
>          configs = []
>          for project in projects:
>              configs.extend(project.configs)
>          for pc in configs:
> -            if locale not in pc.locales:
> +            if locale and locale not in pc.locales:

Nit: if locale is not None and...

Comment 3

[Axel Hecht [:Pike]]

(In reply to Staś Małolepszy :stas from comment #2)
> Comment on attachment 8991897 [details]
> bug 1473577, add validation of reference support,
> 
> https://reviewboard.mozilla.org/r/256810/#review265704
> 
> LGTM. I'm not a fan of en-x-moz-reference, however. The fact that it needs
> to hardcode both "en" and "moz" in there is an indication that perhaps
> there's a better way to go about this. What if --validate wasn't a flag but
> instead took a single locale code as its value? This code could then be used
> to run the plural forms checks.

To explain the choice, it's "en" and a private "moz-reference", and I just wanted to make that discoverable.

For most of the code, None is good enough as magic value, and it's staying clear from all specs. Also, nothing to remember.

Quick overview of alternatives I ditched:

- using a different command. Don't like the idea that people need to remember one vs the other
- using sub-commands. argparse doesn't seem to allow using optional sub commands, didn't want to add more magic manual parsing
- using subclassing of `Files` instead of 'if locale' in the code. Didn't convince me to produce better code either.

Comment 4

[Axel Hecht [:Pike]]

Comment on attachment 8991897 [details]
bug 1473577, add validation of reference support,

https://reviewboard.mozilla.org/r/256810/#review265720

::: compare_locales/compare.py:668
(Diff revision 1)
>          ):
>      locales = set()
>      observers = []
>      for project in project_configs:
> +        # disable filter if we're in validation mode
> +        if None not in project.locales:

Done.

::: compare_locales/paths.py:301
(Diff revision 1)
>          self.mergebase = mergebase
>          configs = []
>          for project in projects:
>              configs.extend(project.configs)
>          for pc in configs:
> -            if locale not in pc.locales:
> +            if locale and locale not in pc.locales:

I typed this, and while it's technically accurate, it makes the logic in the code hard to discover.

Yes, locale being 0 or "" would slip through the cracks, but that's OK :-)

Comment 5

[Axel Hecht [:Pike]]

Thanks.

Marking FIXED, https://hg.mozilla.org/l10n/compare-locales/rev/19a136e82a212b12d4d0db4f81a8fe68fcb44e81.