1321279 - Should be able to apply migrations to existing translations.

Status: RESOLVED FIXED

(Localization Infrastructure and Tools :: Fluent Migration, defect)

Description

[Axel Hecht [:Pike]]

For the heck of it, I ran the menubar migration twice in a row.

The result should be that it's not doing anything on the second run, but it actually regenerates the same sequence of commits and file contents, essentially stripping everything that it did in first migration.

This should start hurting us the first time we want to migrate into an existing file.

Comment 1

[Axel Hecht [:Pike]]

I'm pretty sure we'll hit this soon into the preferences work, in particular if we start with just a small snippet of a single pane.

Looking at the tech stack we have today, I wonder if we could use the compare-locales merge algorithm with the current file as newer, and the migrated content as later content?

Comment 2

[Staś Małolepszy :stas]

(In reply to Axel Hecht [:Pike] from comment #1)
> I'm pretty sure we'll hit this soon into the preferences work, in particular
> if we start with just a small snippet of a single pane.

Why would that be?

> Looking at the tech stack we have today, I wonder if we could use the
> compare-locales merge algorithm with the current file as newer, and the
> migrated content as later content?

We'd need to move the migration code into compare-locales for this. It might generally be a good idea to do so.

Comment 3

[Axel Hecht [:Pike]]

(In reply to Staś Małolepszy :stas from comment #2)
> (In reply to Axel Hecht [:Pike] from comment #1)
> > I'm pretty sure we'll hit this soon into the preferences work, in particular
> > if we start with just a small snippet of a single pane.
> 
> Why would that be?

This bug is really about the migration code resetting the file to just the state of the single migration in each cyle in https://github.com/projectfluent/python-fluent/blob/master/tools/migrate/migrate-l10n.py

I expect us to use an ftl file per pane, as we do now, and if the first migration only converts a fragment of the pane, the next one will hit this bug.

Not on the first migration, but on the one that's following for the same pane.
 
> > Looking at the tech stack we have today, I wonder if we could use the
> > compare-locales merge algorithm with the current file as newer, and the
> > migrated content as later content?
> 
> We'd need to move the migration code into compare-locales for this. It might
> generally be a good idea to do so.

migration already depends on compare-locales, why would we need to move this code into c-l?

I'd just add it to https://github.com/projectfluent/python-fluent/blob/master/tools/migrate/migrate-l10n.py#L52, serialize the newly migrated content (fragment), and merge it with what we have. And then write that actual result to disk.

Comment 4

[Staś Małolepszy :stas]

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

> I expect us to use an ftl file per pane, as we do now, and if the first
> migration only converts a fragment of the pane, the next one will hit this
> bug.
> 
> Not on the first migration, but on the one that's following for the same
> pane.

OK, I see what you mean. I checked the current code and it looks like it doesn't support migrating into an existing localized FTL file. I'm pretty sure I wanted to support this use-case; I must have overlooked it. 

Using compare-locales' merge would indeed fix the symptom. I'd prefer to fix the reason instead :) I look into this today.

> migration already depends on compare-locales, why would we need to move this
> code into c-l?

You're right, thanks.

Comment 5

[Staś Małolepszy :stas]

The migration code is already capable of re-using existing translations from earlier commits. That's how it's able to split the migration across multiple changesets and run them in sequence without the later ones undoing the earlier ones.

Currently migrations need to explicitly read existing FTL files via add_localization() to enable this behavior. Otherwise the migration code assumes the file doesn't exist and recreates it using the en-US template. It starts with an empty file and only merges into it translations from files added via add_localization.

This looks like a usability problem. We could for instance always try to read files at paths specified in add_transforms() so that developers who write migrations don't have to add_localization() them manually.

I'll write tests for this tomorrow to make sure I'm not overlooking anything.

Comment 6

[Staś Małolepszy :stas]

Attachment: Pull Request

This patch changes add_transforms to always try to read the destination FTL file before the migration runs. If found, the FTL file will be used during the merger as the source for translations which should exist according to the the reference template. This prevents migrations from removing existing translations.

I also added a few other usability improvements:

  - I renamed add_localization to maybe_add_localization because source legacy files are not required to exist for the migration to run,
  - migrating from FTL files is not supported and the code now exists early if an FTL file is added via maybe_add_localization,
  - I removed add_reference in favor of passing an additional argument to add_transforms. This removes the redundancy of specifying the destination path both in add_reference and in add_transforms.

Comment 7

[Axel Hecht [:Pike]]

Comment on attachment 8923317 [details] [review]
Pull Request

Left some comments in the PR.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 8923317 [details] [review]
Pull Request

PR has an r+ now.

Comment 9

[Staś Małolepszy :stas]

https://github.com/projectfluent/python-fluent/commit/fee4c63716530526e58995556512338be95dc3d7