Bug 1321279 - Read target FTL files before migrations. #24
Conversation
There was a problem hiding this comment.
This looks generally good, got some inline nits, and, we'll need to adjust https://github.com/stasm/python-fluent/blob/00443015fb5fd0901649bcece2d9b69b1a27ae9e/tools/migrate/migrate-l10n.py#L65 now that a migration might actually not do anything. That should raise an hglib.errors.CommandError when trying to commit without file changes. We want to catch and pass that, I guess?
fluent/migrate/context.py
Outdated
| except UnicodeDecodeError as err: | ||
| logger = logging.getLogger('migrate') | ||
| logger.error('Error reading file {}: {}'.format(refpath, err)) | ||
| sys.exit() |
There was a problem hiding this comment.
Raise a custom exception for these, and then in main(), parser.exit()?
Cleaner, and also makes the exit code become 1.
There was a problem hiding this comment.
Thanks, that's going to be cleaner indeed. I'll still use sys.exit because I don't have access to the ArgumentParser instance inside of main. Also parser.exit is just a wrapper around sys.exit anyways, intended AFAICT for errors about the argument setup.
What do you mean by "pass"? The Python FWIW, here's the output after applying the PR when trying to run the same migrations a second time: No commits are made. Would you like to throw instead? |
stasm
force-pushed
the
1321279-read-ftl-when-migrating
branch
from
0044301
to
e38a550
Compare
There was a problem hiding this comment.
Can you adjust the comment in https://github.com/stasm/python-fluent/blob/e38a55086f66279418d512a9f9fc1f400c8e28a7/fluent/migrate/context.py#L259 to include the case where all our migrations are already in the existing localization?
That's the thing that makes us not even hit the hg commit line now, which I thought we would ;-)
Also, tests/migrate/fixtures/pl/existing.ftl doesn't seem to be used?
Lastly, the tests for NotSupportedError are done once per subclass of Source, should we just have a test_source.py and test that once?
fluent/migrate/merge.py
Outdated
| `in_changeset`; then check for an existing translation in the current FTL | ||
| `localization` or for a migration specification in `transforms`. | ||
| reference, first check for an existing translation in the current FTL | ||
| `localization` and return early if it's present; then if the string has |
There was a problem hiding this comment.
The "return early" here is confusing, because merge_entry returns early, not merge_resource.
|
|
||
| # Migrate an extra string to populate the iterator returned by | ||
| # ctx.merge_changeset(). Otherwise it won't yield anything if the | ||
| # merged contents are the same as the existing file. |
There was a problem hiding this comment.
Looking at how confused I was that that could happen, maybe it's worth explicitly testing that re-applying a transform returns an empty merge_changeset?
stasm
force-pushed
the
1321279-read-ftl-when-migrating
branch
from
95747f1
to
eb9108e
Compare
Read target Fluent localization files before migrating translations into them from legacy resources. This change also explicitly prohibits specifying Fluent resources as sources of migration. They could only be used with the COPY transform anyways.
Remove MergeContext.add_reference in favor of an additional argument to MergeContext.add_transforms. This removes the redundancy of speficying the destination path twice.
stasm
force-pushed
the
1321279-read-ftl-when-migrating
branch
from
eb9108e
to
9de0f5d
Compare
https://bugzilla.mozilla.org/show_bug.cgi?id=1321279