1465342 - Support simple strings.xml files in compare-locales

Status: RESOLVED FIXED

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

Description

[Axel Hecht [:Pike]]

Let's support basic strings.xml files in compare-locales.

Comment 1

[Axel Hecht [:Pike]]

Attachment: bug 1465342, make entities easier to subclass,

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

Comment 2

[Axel Hecht [:Pike]]

Attachment: bug 1465342: Android strings.xml parser and merger based on minidom,

Docs for strings.xml are on https://developer.android.com/guide/topics/resources/string-resource.

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

Comment 3

[Axel Hecht [:Pike]]

Comment on attachment 8981934 [details]
bug 1465342: Android strings.xml parser and merger based on minidom,

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

Comment 4

[Staś Małolepszy :stas]

Comment on attachment 8981933 [details]
bug 1465342, make entities easier to subclass,

https://reviewboard.mozilla.org/r/247934/#review254438

Comment 5

[Staś Małolepszy :stas]

Comment on attachment 8981934 [details]
bug 1465342: Android strings.xml parser and merger based on minidom,

https://reviewboard.mozilla.org/r/247936/#review254440

I have a few questions which might help write some comments :)

::: compare_locales/parser/android.py:17
(Diff revision 2)
> +    EntityBase, Entity, Comment, Junk, Whitespace,
> +    Parser
> +)
> +
> +
> +class AndroidEntity(Entity):

Would it make sense to make AndroidEntity to extends the NodeMixin as well, an override the `key` property?

::: compare_locales/parser/android.py:115
(Diff revision 2)
> +        contents = ctx.contents
> +        try:
> +            doc = minidom.parseString(contents.encode('utf-8'))
> +        except Exception:
> +            yield XMLJunk(contents)
> +            return

Perhaps document the fact that broken XML markup breaks the parsing of the entire file? That's expected in the XML land, but not so much in compare-locales.

::: compare_locales/parser/android.py:121
(Diff revision 2)
> +        if doc.documentElement.nodeName != 'resources':
> +            yield XMLJunk(doc.toxml())
> +            return
> +        root_childs = doc.documentElement.childNodes
> +        yield DocumentWrapper(
> +            '<?xml version="1.0" encoding="utf-8"?>\n<resources>'

Why is it helpful to yield the processing instruction and the root element at all?

::: compare_locales/parser/android.py:123
(Diff revision 2)
> +            return
> +        root_childs = doc.documentElement.childNodes
> +        yield DocumentWrapper(
> +            '<?xml version="1.0" encoding="utf-8"?>\n<resources>'
> +        )
> +        for node in root_childs:

Nit: children :)

::: compare_locales/tests/android/test_parser.py:34
(Diff revision 2)
> +</resources>
> +'''
> +        self._test(
> +            source,
> +            (
> +                (DocumentWrapper, '<?xml'),

Apart from my question above about why this is needed, why is it enough to only check for "<?xml" here rather than the entire processing instruction and the opening tag of the root element?

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 8981934 [details]
bug 1465342: Android strings.xml parser and merger based on minidom,

https://reviewboard.mozilla.org/r/247936/#review254606

Comment 7

[Axel Hecht [:Pike]]

Comment on attachment 8981934 [details]
bug 1465342: Android strings.xml parser and merger based on minidom,

https://reviewboard.mozilla.org/r/247936/#review254440

> Would it make sense to make AndroidEntity to extends the NodeMixin as well, an override the `key` property?

I did remove the .val, as that works now with the Entity patch.

For AndroidEntity, .key isn't the only difference, but hanging on to the node is as well. Right now, we don't use that, but once we support plurals we'll need that, and write checks similar to how we do that for fluent, by digging in to the underlying representation. I don't think it's valuable to do so for the other NodeMixins, and the documentwrapper doesn't even really have an associated node.

> Perhaps document the fact that broken XML markup breaks the parsing of the entire file? That's expected in the XML land, but not so much in compare-locales.

Added a top-level docstring.

> Why is it helpful to yield the processing instruction and the root element at all?

I need that content for the merge algorithm.

> Nit: children :)

"I was dreaming when I wrote this, forgive if it goes astray", sung to a tune by Prince.

> Apart from my question above about why this is needed, why is it enough to only check for "<?xml" here rather than the entire processing instruction and the opening tag of the root element?

I'm not trying to preserve the original string, so it's good enough to just check for "an xml header". Mostly "don't test hard-coded strings pedantically".

Comment 8

[Staś Małolepszy :stas]

Comment on attachment 8981934 [details]
bug 1465342: Android strings.xml parser and merger based on minidom,

https://reviewboard.mozilla.org/r/247936/#review254618

::: compare_locales/tests/android/test_parser.py:34
(Diff revision 2)
> +</resources>
> +'''
> +        self._test(
> +            source,
> +            (
> +                (DocumentWrapper, '<?xml'),

Oh, I see. I didn't realize ParserTestMixin used self.assertIn for non-entities.

Comment 9

[Axel Hecht [:Pike]]

Yeah, it's one of those things :-)

https://hg.mozilla.org/l10n/compare-locales/rev/c26b6be554a410b5c8ba582fc15eb7ff7d392013
https://hg.mozilla.org/l10n/compare-locales/rev/9d7f6c7bb19bf657ec804e57924009014087d313

Marking FIXED.