1399059 - [compare-locales] Optionally yield Whitespace between Entities in Parser.walk()

Status: RESOLVED FIXED

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

Description

[Staś Małolepszy :stas]

We should unify how compare-locales' Parsers handle Whitespace in legacy and Fluent resources. Axel suggested refactoring the whole thing and yielding all inter-Entity whitespace as new Whitespace instances.

Comment 1

[Staś Małolepszy :stas]

Attachment: Bug 1399059 - Part 1 - Yield Whitespace from FluentParser.

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

Comment 2

[Staś Małolepszy :stas]

Attachment: Bug 1399059 - Part 2 - Remove pre_ws, post spans and getters.

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

Comment 3

[Staś Małolepszy :stas]

Attachment: Bug 1399059 - Part 3 - Don't return offset from getEntity.

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

Comment 4

[Staś Małolepszy :stas]

Attachment: Bug 1399059 - Part 4 - Parser.walk(withWhitespace=False).

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

Comment 5

[Staś Małolepszy :stas]

Attachment: Bug 1399059 - Yield Whitespace between Entities in Parser.walk(only_localizable=False).

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

Comment 6

[Staś Małolepszy :stas]

Attachment: Bug 1399059 - Part 6 - Return Whitespace from getNext (previously called getEntity).

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

Comment 7

[Staś Małolepszy :stas]

Attachment: Bug 1399059 - Part 7 - Fold onlyEntities and withWhitespace into only_localizable.

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

Comment 8

[Staś Małolepszy :stas]

Attachment: Bug 1399059 - Part 8 - Use named regex groups.

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

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 8910849 [details]
Bug 1399059 - Part 1 - Yield Whitespace from FluentParser.

https://reviewboard.mozilla.org/r/182322/#review187864

::: compare_locales/tests/test_dtd.py:131
(Diff revision 1)
>  escaped value">
>  ''')
>          one, two = list(self.parser)
>          self.assertEqual(one.position(), (1, 1))
>          self.assertEqual(one.value_position(), (1, 16))
> -        self.assertEqual(one.position(-1), (2, 1))
> +        self.assertEqual(one.position(-1), (1, 23))

That looks like a bug, the `one` entity should start on the 1 row of line 2

::: compare_locales/tests/test_merge.py:298
(Diff revision 1)
>               'details': {
>                   'l10n.dtd': [
>                       {'error': u'Unparsed content "<!ENTY bar '
> -                               u'\'gimmick\'>" '
> +                               u'\'gimmick\'>\n" '
>                                 u'from line 2 column 1 to '
> -                               u'line 2 column 22'},
> +                               u'line 3 column 1'},

Another error position that probably shouldn't change.

Comment 10

[Staś Małolepszy :stas]

Comment on attachment 8910853 [details]
Bug 1399059 - Yield Whitespace between Entities in Parser.walk(only_localizable=False).

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

Comment 11

[Staś Małolepszy :stas]

(In reply to Axel Hecht [:Pike] from comment #9)

> That looks like a bug, the `one` entity should start on the 1 row of line 2
> Another error position that probably shouldn't change.

Both of these changes are related to the fact that Entities don't include the trailing newline anymore and Junk does include all the trailing whitespace. Also, `position(-1)` returns the end position of the entity.

Comment 12

[Staś Małolepszy :stas]

Comment on attachment 8910853 [details]
Bug 1399059 - Yield Whitespace between Entities in Parser.walk(only_localizable=False).

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

Comment 13

[Staś Małolepszy :stas]

Comment on attachment 8910853 [details]
Bug 1399059 - Yield Whitespace between Entities in Parser.walk(only_localizable=False).

https://reviewboard.mozilla.org/r/182332/#review188144

::: compare_locales/tests/test_ftl.py:155
(Diff revision 3)
> +baz = Baz
> +''')
> +        entities = list(self.parser.walk())
> +
> +        self.assertTrue(isinstance(entities[0], parser.Whitespace))
> +        self.assertEqual(entities[0].all, '\n')

python-fluent parses comments without the trailing newline but the corresponding span is one char too long.  That's why there's only one \n here. I filed bug 1402600.

Comment 14

[Axel Hecht [:Pike]]

Comment on attachment 8910853 [details]
Bug 1399059 - Yield Whitespace between Entities in Parser.walk(only_localizable=False).

https://reviewboard.mozilla.org/r/182332/#review187904

Sweet, thanks. Only one nit about removing that left-over comment for DTDEntity completely, r=me with that.

::: compare_locales/parser.py:307
(Diff revision 2)
> -# 6: post comment (and white space) in the same line (dtd only)
> -#                                            <--[1]
> +# <!-- pre comments -->                      <--[1]
> +# <!ENTITY key "value">
> -# <!-- pre comments -->                      <--[2]
> -# <!ENTITY key "value"> <!-- comment -->
>  #
> -# <-------[3]---------><------[6]------>
> +# <-------[2]--------->

I've reread this after we talked about this. I think we should just remove this comment alltogether, there's no point in having it here anymore.

Comment 15

[Staś Małolepszy :stas]

Comment on attachment 8910853 [details]
Bug 1399059 - Yield Whitespace between Entities in Parser.walk(only_localizable=False).

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

Comment 16

[Staś Małolepszy :stas]

https://hg.mozilla.org/l10n/compare-locales/rev/9d38b940dff1bdca5f08fe2fd053324f6d828346
Bug 1399059 - Yield Whitespace between Entities in Parser.walk(only_localizable=False). r=Pike

Comment 17

[Staś Małolepszy :stas]

It is done.

Comment 18

[Francesco Lodolo [:flod]]

This patch just made compare-locales unusable. I get over 20 warnings, most of them in en-US, and weirdly enough they seem to happen when the string contains a Z.

Comment 19

[Staś Małolepszy :stas]

Oops. Sorry. The problem is that I used \Z in a character class expression: 

    self._trailingWS = re.compile(r'\s*[\n\Z])', re.M)

Which is wrong because it's not a character. The fix is to use a group:

    self._trailingWS = re.compile(r'\s*(?:\n|\Z)', re.M)

Comment 20

[Staś Małolepszy :stas]

https://hg.mozilla.org/l10n/compare-locales/rev/63748ff0760ad890782778986f2a839b602274a5
Bug 1399059 - Follow-up fix for \Z. r=Pike