1280894 - [life] support ftl/l20n in diff mode

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect)

Description

[Axel Hecht [:Pike]]

We need support for ftl in elmo to use our smart diffs for that.

I'm not sure we need to support complex values for this.

Comment 1

[Axel Hecht [:Pike]]

Mass change dependency tree for bug 1279002 into a whiteboard keyword.

Comment 2

[Axel Hecht [:Pike]]

Thinking about this a bit:

The easiest start here will be to treat messages as key-value, and switch on white-space: pre-wrap; for those, so that we get significant whitespace support.

There are a few scenarios to support, I guess:

- new and removed messages would work like now, might be good to test that still
- We should have extra tests for internal changes to the structure
-- add/remove/modify value
-- add/remove/modify trait

Comment 3

[Axel Hecht [:Pike]]

Gandalf said he'll work on this, assigning to him.

Regarding comment 2, I think with the changes to the fluent syntax, we can create a diff line for value and a diff line per attribute.

Looking at the code in apps/pushes/views/diff.py, it might make sense to update compare-locales in elmo first, in particular to pick up the changes from bug 1368444.

Comment 4

[Axel Hecht [:Pike]]

I filed bug 1372257 on the c-l update.

Comment 5

[Axel Hecht [:Pike]]

Turns out that it'll be easier for me to just grab this and do the corresponding jazz in compare-locales and elmo that I talked to stas about.

We'll introduce a lazy EntityBase subclass for Attributes to compare with.

Looking at the code it'll actually be easier to stick to compare-locales parser wrapper than trying to detect fluent explicitly before parsing.

Comment 6

[Axel Hecht [:Pike]]

Attachment: bug 1280894, expose attributes on FluentEntity,

For the elmo diff, it's going to be nice to have access to a
wrapper for Attribute nodes.

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

Comment 7

[Axel Hecht [:Pike]]

Attachment: bug 1280894, make AddRemove sort stable without map hacks,

The logic to have ordering for AddRemove given externally was
a hack, in hindsight.
This patch makes AddRemove stable given two iterables, and adds
tests to verify that.
The test fixes are OK, as the ordering there is for the same
position in the localization, and it's kinda ambiguous which
should come first or not.
This is in support for elmo fluent diffs, 'cause then I
can use AddRemove easier there for both keys and attr names.

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

Comment 8

[Staś Małolepszy :stas]

Comment on attachment 8882128 [details]
bug 1280894, expose attributes on FluentEntity,

https://reviewboard.mozilla.org/r/153246/#review158422

Comment 9

[Staś Małolepszy :stas]

Comment on attachment 8882129 [details]
bug 1280894, make AddRemove sort stable without map hacks,

https://reviewboard.mozilla.org/r/153248/#review158430

Comment 10

[Axel Hecht [:Pike]]

Comment on attachment 8882128 [details]
bug 1280894, expose attributes on FluentEntity,

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

Comment 11

[Axel Hecht [:Pike]]

Comment on attachment 8882129 [details]
bug 1280894, make AddRemove sort stable without map hacks,

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

Comment 12

[Axel Hecht [:Pike]]

Attachment: pull request for this and bug 1372257

I'm moving this into one PR for both this and bug 1372257.

Review is on stas, per conversation IRL.

Comment 13

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/e40c6b24ec5271dbfde8c6740c67747baaaa836c
bug 1280894, add diff support for fluent, r=stas

Also, update compare-locales once more to get better support
from AddRemove

Comment 14

[Axel Hecht [:Pike]]

Landed. Will deploy as soon as we have a few other commits.

Comment 15

[Axel Hecht [:Pike]]

This is deployed on both stage and prod now.