[life] support ftl/l20n in diff mode

RESOLVED FIXED

Status

Webtools
Elmo
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Pike, Assigned: Pike)

Tracking

Trunk

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gecko-l20n])

User Story

Diff support for ftl/l20n in elmo.

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Mass change dependency tree for bug 1279002 into a whiteboard keyword.
No longer blocks: 1279002
Whiteboard: [gecko-l20n]
(Assignee)

Comment 2

2 years ago
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
Assignee: nobody → l10n
(Assignee)

Comment 3

a year ago
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.
Assignee: l10n → gandalf
(Assignee)

Comment 4

a year ago
I filed bug 1372257 on the c-l update.
(Assignee)

Comment 5

a year ago
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.
Assignee: gandalf → l10n
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

a year ago
mozreview-review
Comment on attachment 8882128 [details]
bug 1280894, expose attributes on FluentEntity,

https://reviewboard.mozilla.org/r/153246/#review158422
Attachment #8882128 - Flags: review?(stas) → review+

Comment 9

a year ago
mozreview-review
Comment on attachment 8882129 [details]
bug 1280894, make AddRemove sort stable without map hacks,

https://reviewboard.mozilla.org/r/153248/#review158430
Attachment #8882129 - Flags: review?(stas) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
Created attachment 8882160 [details] [review]
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

a year ago
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
(Assignee)

Comment 14

a year ago
Landed. Will deploy as soon as we have a few other commits.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
(Assignee)

Comment 15

a year ago
This is deployed on both stage and prod now.
You need to log in before you can comment on or make changes to this bug.