1371601 - compare-locales should error on duplicate entries in l10n, warn for en-US

Status: RESOLVED FIXED

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

Description

[Axel Hecht [:Pike]]

We don't show anything right now if an entry shows up more than once, we should error for l10n, and warn for en-US.

Comment 1

[Axel Hecht [:Pike]]

Attachment: bug 1371601, prep: make variable names for reference entities nicer,

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

Comment 2

[Axel Hecht [:Pike]]

Attachment: bug 1371601, check for duplicate entities, and duplicate attributes in ftl,

For Entities, en-US duplicates are warnings, and l10n duplicates are errors.
For ftl attributes, ignore en-US duplicates alltogether, and just warn
on duplicate attributes on l10n. That way, we don't end up skipping the translated
message.

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

Comment 3

[Staś Małolepszy :stas]

Comment on attachment 8881733 [details]
bug 1371601, prep: make variable names for reference entities nicer,

https://reviewboard.mozilla.org/r/152844/#review157980

::: compare_locales/compare.py:480
(Diff revision 1)
> -        checker = getChecker(l10n, reference=ref[0], extra_tests=extra_tests)
> +        checker = getChecker(l10n,
> +                             reference=ref_entities, extra_tests=extra_tests)
>          for action, entity in ar:
>              if action == 'delete':
>                  # missing entity
> -                if isinstance(ref[0][ref[1][entity]], parser.Junk):
> +                if isinstance(ref_entities[ref_map[entity]], parser.Junk):

To make this clearer, how about renaming `entity` to `key` and then:

    entity = ref_entities[ref_map[key]]
    
right after the `for … in`?

Comment 4

[Staś Małolepszy :stas]

Comment on attachment 8881734 [details]
bug 1371601, check for duplicate entities, and duplicate attributes in ftl,

https://reviewboard.mozilla.org/r/152846/#review157982

::: compare_locales/parser.py:321
(Diff revision 1)
> +        found = defaultdict(list)
> +        for entity in entities:
> +            found[entity.key].append(entity)
> +        for entity_list in found.values():
> +            if len(entity_list) > 1:
> +                yield '{} occurs {} times'.format(

If you don't plan to add more data to the reported error/warning here, perhaps also use Counter here?

Comment 5

[Axel Hecht [:Pike]]

Comment on attachment 8881733 [details]
bug 1371601, prep: make variable names for reference entities nicer,

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

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 8881734 [details]
bug 1371601, check for duplicate entities, and duplicate attributes in ftl,

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

Comment 7

[Axel Hecht [:Pike]]

remote:   https://hg.mozilla.org/l10n/compare-locales/rev/a5d2a6476e667c61fbb8c180df14b2f75f9ef2b1
remote:   https://hg.mozilla.org/l10n/compare-locales/rev/0dcd32f2781955a4b8f63e680af8de4d901497cc

Resolving FIXED, there are a couple of follow-ups for duplicate attributes and message references.