From 52a909afbaa408dd4accffb24ade415a21146df5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sta=C5=9B=20Ma=C5=82olepszy?= Date: Fri, 27 Oct 2017 15:04:00 +0200 Subject: [PATCH 1/2] Bug 1321279 - Part 1 - Read target FTL files before migrations. 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. --- fluent/migrate/__init__.py | 3 + fluent/migrate/context.py | 108 +++++++++------- fluent/migrate/errors.py | 10 ++ fluent/migrate/merge.py | 7 +- fluent/migrate/transforms.py | 6 + tests/migrate/fixtures/en-US/privacy.ftl | 9 ++ tests/migrate/fixtures/pl/privacy.dtd | 7 ++ tests/migrate/fixtures/pl/privacy.ftl | 5 + tests/migrate/test_concat.py | 36 +++--- tests/migrate/test_context.py | 130 +++++++++++++++++++- tests/migrate/test_context_real_examples.py | 6 +- tests/migrate/test_copy.py | 18 +-- tests/migrate/test_merge.py | 18 +-- tests/migrate/test_plural.py | 8 +- tests/migrate/test_replace.py | 14 ++- tests/migrate/test_source.py | 51 ++++++++ tools/migrate/examples/about_dialog.py | 2 +- tools/migrate/examples/about_downloads.py | 6 +- tools/migrate/examples/bug_1291693.py | 8 +- tools/migrate/migrate-l10n.py | 21 +++- 20 files changed, 364 insertions(+), 109 deletions(-) create mode 100644 fluent/migrate/errors.py create mode 100644 tests/migrate/fixtures/en-US/privacy.ftl create mode 100644 tests/migrate/fixtures/pl/privacy.dtd create mode 100644 tests/migrate/fixtures/pl/privacy.ftl create mode 100644 tests/migrate/test_source.py diff --git a/fluent/migrate/__init__.py b/fluent/migrate/__init__.py index 2a92dd5e..3582468d 100644 --- a/fluent/migrate/__init__.py +++ b/fluent/migrate/__init__.py @@ -1,6 +1,9 @@ # coding=utf8 from .context import MergeContext # noqa: F401 +from .errors import ( # noqa: F401 + MigrationError, NotSupportedError, UnreadableReferenceError +) from .transforms import ( # noqa: F401 Source, COPY, REPLACE_IN_TEXT, REPLACE, PLURALS, CONCAT ) diff --git a/fluent/migrate/context.py b/fluent/migrate/context.py index 4c8a8c44..8c800589 100644 --- a/fluent/migrate/context.py +++ b/fluent/migrate/context.py @@ -18,7 +18,7 @@ def getParser(path): from .cldr import get_plural_categories from .transforms import Source from .merge import merge_resource -from .util import get_message +from .errors import NotSupportedError, UnreadableReferenceError class MergeContext(object): @@ -79,6 +79,10 @@ def read_ftl_resource(self, path): f = codecs.open(path, 'r', 'utf8') try: contents = f.read() + except UnicodeDecodeError as err: + logger = logging.getLogger('migrate') + logger.warn('Unable to read file {}: {}'.format(path, err)) + raise err finally: f.close() @@ -94,7 +98,7 @@ def read_ftl_resource(self, path): logger = logging.getLogger('migrate') for annot in annots: msg = annot.message - logger.warn(u'Syntax error in {}: {}'.format(path, msg)) + logger.warn('Syntax error in {}: {}'.format(path, msg)) return ast @@ -111,43 +115,40 @@ def add_reference(self, path, realpath=None): try: ast = self.read_ftl_resource(fullpath) except IOError as err: - logger = logging.getLogger('migrate') - logger.error(u'Missing reference file: {}'.format(path)) - raise err + error_message = 'Missing reference file: {}'.format(fullpath) + logging.getLogger('migrate').error(error_message) + raise UnreadableReferenceError(error_message) except UnicodeDecodeError as err: - logger = logging.getLogger('migrate') - logger.error(u'Error reading file {}: {}'.format(path, err)) - raise err + error_message = 'Error reading file {}: {}'.format(fullpath, err) + logging.getLogger('migrate').error(error_message) + raise UnreadableReferenceError(error_message) else: self.reference_resources[path] = ast - def add_localization(self, path): - """Add an existing localization resource. + def maybe_add_localization(self, path): + """Add a localization resource to migrate translations from. - If it's an FTL resource, add an FTL AST. Otherwise, it's a legacy - resource. Use a compare-locales parser to create a dict of (key, - string value) tuples. + Only legacy resources can be added as migration sources. The resource + may be missing on disk. + + Uses a compare-locales parser to create a dict of (key, string value) + tuples. """ - fullpath = os.path.join(self.localization_dir, path) - if fullpath.endswith('.ftl'): - try: - ast = self.read_ftl_resource(fullpath) - except IOError: - logger = logging.getLogger('migrate') - logger.warn(u'Missing localization file: {}'.format(path)) - except UnicodeDecodeError as err: - logger = logging.getLogger('migrate') - logger.warn(u'Error reading file {}: {}'.format(path, err)) - else: - self.localization_resources[path] = ast + if path.endswith('.ftl'): + error_message = ( + 'Migrating translations from Fluent files is not supported ' + '({})'.format(path)) + logging.getLogger('migrate').error(error_message) + raise NotSupportedError(error_message) + + try: + fullpath = os.path.join(self.localization_dir, path) + collection = self.read_legacy_resource(fullpath) + except IOError: + logger = logging.getLogger('migrate') + logger.warn('Missing localization file: {}'.format(path)) else: - try: - collection = self.read_legacy_resource(fullpath) - except IOError: - logger = logging.getLogger('migrate') - logger.warn(u'Missing localization file: {}'.format(path)) - else: - self.localization_resources[path] = collection + self.localization_resources[path] = collection def add_transforms(self, path, transforms): """Define transforms for path. @@ -175,17 +176,30 @@ def get_sources(acc, cur): path_transforms = self.transforms.setdefault(path, []) path_transforms += transforms + if path not in self.localization_resources: + fullpath = os.path.join(self.localization_dir, path) + try: + ast = self.read_ftl_resource(fullpath) + except IOError: + logger = logging.getLogger('migrate') + logger.info( + 'Localization file {} does not exist and ' + 'it will be created'.format(path)) + except UnicodeDecodeError: + logger = logging.getLogger('migrate') + logger.warn( + 'Localization file {} will be re-created and some ' + 'translations might be lost'.format(path)) + else: + self.localization_resources[path] = ast + def get_source(self, path, key): - """Get an entity value from the localized source. + """Get an entity value from a localized legacy source. Used by the `Source` transform. """ - if path.endswith('.ftl'): - resource = self.localization_resources[path] - return get_message(resource.body, key) - else: - resource = self.localization_resources[path] - return resource.get(key, None) + resource = self.localization_resources[path] + return resource.get(key, None) def merge_changeset(self, changeset=None): """Return a generator of FTL ASTs for the changeset. @@ -200,10 +214,11 @@ def merge_changeset(self, changeset=None): """ if changeset is None: - # Merge all known legacy translations. + # Merge all known legacy translations. Used in tests. changeset = { (path, key) for path, strings in self.localization_resources.iteritems() + if not path.endswith('.ftl') for key in strings.iterkeys() } @@ -240,10 +255,15 @@ def in_changeset(ident): self, reference, current, transforms, in_changeset ) - # If none of the transforms is in the given changeset, the merged - # snapshot is identical to the current translation. We compare - # JSON trees rather then use filtering by `in_changeset` to account - # for translations removed from `reference`. + # Skip this path if the merged snapshot is identical to the current + # state of the localization file. This may happen when: + # + # - none of the transforms is in the changset, or + # - all messages which would be migrated by the context's + # transforms already exist in the current state. + # + # We compare JSON trees rather then use filtering by `in_changeset` + # to account for translations removed from `reference`. if snapshot.to_json() == current.to_json(): continue diff --git a/fluent/migrate/errors.py b/fluent/migrate/errors.py new file mode 100644 index 00000000..e52984f0 --- /dev/null +++ b/fluent/migrate/errors.py @@ -0,0 +1,10 @@ +class MigrationError(ValueError): + pass + + +class NotSupportedError(MigrationError): + pass + + +class UnreadableReferenceError(MigrationError): + pass diff --git a/fluent/migrate/merge.py b/fluent/migrate/merge.py index 3112cb54..586393e5 100644 --- a/fluent/migrate/merge.py +++ b/fluent/migrate/merge.py @@ -11,9 +11,10 @@ def merge_resource(ctx, reference, current, transforms, in_changeset): """Transform legacy translations into FTL. Use the `reference` FTL AST as a template. For each en-US string in the - reference, first check if it's in the currently processed changeset with - `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 use it if it's present; then if the string has + a transform defined in the migration specification and if it's in the + currently processed changeset, evaluate the transform. """ def merge_body(body): diff --git a/fluent/migrate/transforms.py b/fluent/migrate/transforms.py index 942fbd90..b747a128 100644 --- a/fluent/migrate/transforms.py +++ b/fluent/migrate/transforms.py @@ -66,6 +66,7 @@ from __future__ import unicode_literals import fluent.syntax.ast as FTL +from .errors import NotSupportedError def pattern_from_text(value): @@ -113,6 +114,11 @@ class Source(Transform): # not be replaced? def __init__(self, path, key): + if path.endswith('.ftl'): + raise NotSupportedError( + 'Migrating translations from Fluent files is not supported ' + '({})'.format(path)) + self.path = path self.key = key diff --git a/tests/migrate/fixtures/en-US/privacy.ftl b/tests/migrate/fixtures/en-US/privacy.ftl new file mode 100644 index 00000000..bedd5a7c --- /dev/null +++ b/tests/migrate/fixtures/en-US/privacy.ftl @@ -0,0 +1,9 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +dnt-description = + Send websites a “Do Not Track” signal + that you don’t want to be tracked +dnt-learn-more = Learn more +dnt-always = Always diff --git a/tests/migrate/fixtures/pl/privacy.dtd b/tests/migrate/fixtures/pl/privacy.dtd new file mode 100644 index 00000000..36878d2f --- /dev/null +++ b/tests/migrate/fixtures/pl/privacy.dtd @@ -0,0 +1,7 @@ + + + + + diff --git a/tests/migrate/fixtures/pl/privacy.ftl b/tests/migrate/fixtures/pl/privacy.ftl new file mode 100644 index 00000000..772da74e --- /dev/null +++ b/tests/migrate/fixtures/pl/privacy.ftl @@ -0,0 +1,5 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +dnt-description = New Description in Polish diff --git a/tests/migrate/test_concat.py b/tests/migrate/test_concat.py index e63f41c2..48a4626e 100644 --- a/tests/migrate/test_concat.py +++ b/tests/migrate/test_concat.py @@ -16,6 +16,8 @@ class MockContext(unittest.TestCase): def get_source(self, path, key): + # Ignore path (test.properties) and get translations from self.strings + # defined in setUp. return self.strings.get(key, None).val @@ -36,7 +38,7 @@ def test_concat_one(self): msg = FTL.Message( FTL.Identifier('hello'), value=CONCAT( - COPY(self.strings, 'hello'), + COPY('test.properties', 'hello'), ) ) @@ -51,8 +53,8 @@ def test_concat_two(self): msg = FTL.Message( FTL.Identifier('hello'), value=CONCAT( - COPY(self.strings, 'hello.start'), - COPY(self.strings, 'hello.end'), + COPY('test.properties', 'hello.start'), + COPY('test.properties', 'hello.end'), ) ) @@ -86,8 +88,8 @@ def test_concat_whitespace_begin(self): msg = FTL.Message( FTL.Identifier('hello'), value=CONCAT( - COPY(self.strings, 'whitespace.begin.start'), - COPY(self.strings, 'whitespace.begin.end'), + COPY('test.properties', 'whitespace.begin.start'), + COPY('test.properties', 'whitespace.begin.end'), ) ) @@ -103,8 +105,8 @@ def test_concat_whitespace_end(self): msg = FTL.Message( FTL.Identifier('hello'), value=CONCAT( - COPY(self.strings, 'whitespace.end.start'), - COPY(self.strings, 'whitespace.end.end'), + COPY('test.properties', 'whitespace.end.start'), + COPY('test.properties', 'whitespace.end.end'), ) ) @@ -129,11 +131,11 @@ def test_concat_literal(self): msg = FTL.Message( FTL.Identifier('update-failed'), value=CONCAT( - COPY(self.strings, 'update.failed.start'), + COPY('test.properties', 'update.failed.start'), FTL.TextElement(''), - COPY(self.strings, 'update.failed.linkText'), + COPY('test.properties', 'update.failed.linkText'), FTL.TextElement(''), - COPY(self.strings, 'update.failed.end'), + COPY('test.properties', 'update.failed.end'), ) ) @@ -157,9 +159,9 @@ def test_concat_replace(self): msg = FTL.Message( FTL.Identifier('channel-desc'), value=CONCAT( - COPY(self.strings, 'channel.description.start'), + COPY('test.properties', 'channel.description.start'), FTL.Placeable(EXTERNAL_ARGUMENT('channelname')), - COPY(self.strings, 'channel.description.end'), + COPY('test.properties', 'channel.description.end'), ) ) @@ -187,7 +189,7 @@ def test_concat_replace(self): FTL.Identifier('community'), value=CONCAT( REPLACE( - self.strings, + 'test.properties', 'community.start', { '&brandShortName;': MESSAGE_REFERENCE( @@ -197,7 +199,7 @@ def test_concat_replace(self): ), FTL.TextElement(''), REPLACE( - self.strings, + 'test.properties', 'community.mozillaLink', { '&vendorShortName;': MESSAGE_REFERENCE( @@ -206,11 +208,11 @@ def test_concat_replace(self): } ), FTL.TextElement(''), - COPY(self.strings, 'community.middle'), + COPY('test.properties', 'community.middle'), FTL.TextElement(''), - COPY(self.strings, 'community.creditsLink'), + COPY('test.properties', 'community.creditsLink'), FTL.TextElement(''), - COPY(self.strings, 'community.end') + COPY('test.properties', 'community.end') ) ) diff --git a/tests/migrate/test_context.py b/tests/migrate/test_context.py index 775db8f2..0016034e 100644 --- a/tests/migrate/test_context.py +++ b/tests/migrate/test_context.py @@ -7,6 +7,7 @@ import fluent.syntax.ast as FTL +from fluent.migrate.errors import NotSupportedError, UnreadableReferenceError from fluent.migrate.util import ftl, ftl_resource_to_json, to_json from fluent.migrate.context import MergeContext from fluent.migrate.transforms import COPY @@ -27,8 +28,8 @@ def setUp(self): self.ctx.add_reference('aboutDownloads.ftl') try: - self.ctx.add_localization('aboutDownloads.dtd') - self.ctx.add_localization('aboutDownloads.properties') + self.ctx.maybe_add_localization('aboutDownloads.dtd') + self.ctx.maybe_add_localization('aboutDownloads.properties') except RuntimeError: self.skipTest('compare-locales required') @@ -245,7 +246,7 @@ def tearDown(self): logging.disable(logging.NOTSET) def test_missing_reference_file(self): - with self.assertRaises(IOError): + with self.assertRaises(UnreadableReferenceError): self.ctx.add_reference('missing.ftl') @@ -262,7 +263,7 @@ def setUp(self): self.ctx.add_reference('toolbar.ftl') try: - self.ctx.add_localization('browser.dtd') + self.ctx.maybe_add_localization('browser.dtd') except RuntimeError: self.skipTest('compare-locales required') @@ -309,3 +310,124 @@ def test_missing_localization_file(self): to_json(self.ctx.merge_changeset()), expected ) + + +class TestExistingTarget(unittest.TestCase): + def setUp(self): + # Silence all logging. + logging.disable(logging.CRITICAL) + + self.ctx = MergeContext( + lang='pl', + reference_dir=here('fixtures/en-US'), + localization_dir=here('fixtures/pl') + ) + + self.ctx.add_reference('privacy.ftl') + try: + self.ctx.maybe_add_localization('privacy.dtd') + except RuntimeError: + self.skipTest('compare-locales required') + + def tearDown(self): + # Resume logging. + logging.disable(logging.NOTSET) + + def test_existing_target_ftl_missing_string(self): + self.ctx.add_transforms('privacy.ftl', [ + FTL.Message( + id=FTL.Identifier('dnt-learn-more'), + value=COPY( + 'privacy.dtd', + 'doNotTrack.learnMore.label' + ) + ), + ]) + + expected = { + 'privacy.ftl': ftl_resource_to_json(''' + // This Source Code Form is subject to the terms of the Mozilla Public + // License, v. 2.0. If a copy of the MPL was not distributed with this + // file, You can obtain one at http://mozilla.org/MPL/2.0/. + + dnt-description = New Description in Polish + dnt-learn-more = Więcej informacji + ''') + } + + self.maxDiff = None + self.assertDictEqual( + to_json(self.ctx.merge_changeset()), + expected + ) + + def test_existing_target_ftl_existing_string(self): + self.ctx.add_transforms('privacy.ftl', [ + FTL.Message( + id=FTL.Identifier('dnt-description'), + value=COPY( + 'privacy.dtd', + 'doNotTrack.description' + ) + ), + + # 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. + FTL.Message( + id=FTL.Identifier('dnt-always'), + value=COPY( + 'privacy.dtd', + 'doNotTrack.always.label' + ) + ), + ]) + + expected = { + 'privacy.ftl': ftl_resource_to_json(''' + // This Source Code Form is subject to the terms of the Mozilla Public + // License, v. 2.0. If a copy of the MPL was not distributed with this + // file, You can obtain one at http://mozilla.org/MPL/2.0/. + + dnt-description = New Description in Polish + dnt-always = Zawsze + ''') + } + + self.maxDiff = None + self.assertDictEqual( + to_json(self.ctx.merge_changeset()), + expected + ) + + def test_existing_target_ftl_with_all_messages(self): + self.ctx.add_transforms('privacy.ftl', [ + FTL.Message( + id=FTL.Identifier('dnt-description'), + value=COPY( + 'privacy.dtd', + 'doNotTrack.description' + ) + ), + ]) + + # All migrated messages are already in the target FTL and the result of + # merge_changeset is an empty iterator. + self.assertDictEqual( + to_json(self.ctx.merge_changeset()), + {} + ) + + +class TestNotSupportedError(unittest.TestCase): + def test_add_ftl(self): + pattern = ('Migrating translations from Fluent files is not supported') + with self.assertRaisesRegexp(NotSupportedError, pattern): + ctx = MergeContext( + lang='pl', + reference_dir=here('fixtures/en-US'), + localization_dir=here('fixtures/pl') + ) + + ctx.add_reference('privacy.ftl') + ctx.maybe_add_localization('privacy.ftl') diff --git a/tests/migrate/test_context_real_examples.py b/tests/migrate/test_context_real_examples.py index 7953885e..06bada70 100644 --- a/tests/migrate/test_context_real_examples.py +++ b/tests/migrate/test_context_real_examples.py @@ -29,8 +29,8 @@ def setUp(self): self.ctx.add_reference('aboutDownloads.ftl') try: - self.ctx.add_localization('aboutDownloads.dtd') - self.ctx.add_localization('aboutDownloads.properties') + self.ctx.maybe_add_localization('aboutDownloads.dtd') + self.ctx.maybe_add_localization('aboutDownloads.properties') except RuntimeError: self.skipTest('compare-locales required') @@ -290,7 +290,7 @@ def setUp(self): try: self.ctx.add_reference('aboutDialog.ftl') - self.ctx.add_localization('aboutDialog.dtd') + self.ctx.maybe_add_localization('aboutDialog.dtd') except RuntimeError: self.skipTest('compare-locales required') diff --git a/tests/migrate/test_copy.py b/tests/migrate/test_copy.py index 55e78a20..66eb8c85 100644 --- a/tests/migrate/test_copy.py +++ b/tests/migrate/test_copy.py @@ -15,6 +15,8 @@ class MockContext(unittest.TestCase): def get_source(self, path, key): + # Ignore path (test.properties) and get translations from self.strings + # defined in setUp. return self.strings.get(key, None).val @@ -33,7 +35,7 @@ def setUp(self): def test_copy(self): msg = FTL.Message( FTL.Identifier('foo'), - value=COPY(self.strings, 'foo') + value=COPY('test.properties', 'foo') ) self.assertEqual( @@ -46,7 +48,7 @@ def test_copy(self): def test_copy_escape_unicode_middle(self): msg = FTL.Message( FTL.Identifier('foo-unicode-middle'), - value=COPY(self.strings, 'foo.unicode.middle') + value=COPY('test.properties', 'foo.unicode.middle') ) self.assertEqual( @@ -60,7 +62,7 @@ def test_copy_escape_unicode_middle(self): def test_copy_escape_unicode_begin(self): msg = FTL.Message( FTL.Identifier('foo-unicode-begin'), - value=COPY(self.strings, 'foo.unicode.begin') + value=COPY('test.properties', 'foo.unicode.begin') ) self.assertEqual( @@ -74,7 +76,7 @@ def test_copy_escape_unicode_begin(self): def test_copy_escape_unicode_end(self): msg = FTL.Message( FTL.Identifier('foo-unicode-end'), - value=COPY(self.strings, 'foo.unicode.end') + value=COPY('test.properties', 'foo.unicode.end') ) self.assertEqual( @@ -87,7 +89,7 @@ def test_copy_escape_unicode_end(self): def test_copy_html_entity(self): msg = FTL.Message( FTL.Identifier('foo-html-entity'), - value=COPY(self.strings, 'foo.html.entity') + value=COPY('test.properties', 'foo.html.entity') ) self.assertEqual( @@ -99,7 +101,7 @@ def test_copy_html_entity(self): @unittest.skipUnless(DTDParser, 'compare-locales required') -class TestCopyTraits(MockContext): +class TestCopyAttributes(MockContext): def setUp(self): self.strings = parse(DTDParser, ''' @@ -112,12 +114,12 @@ def test_copy_accesskey(self): attributes=[ FTL.Attribute( FTL.Identifier('label'), - COPY(self.strings, 'checkForUpdatesButton.label') + COPY('test.properties', 'checkForUpdatesButton.label') ), FTL.Attribute( FTL.Identifier('accesskey'), COPY( - self.strings, 'checkForUpdatesButton.accesskey' + 'test.properties', 'checkForUpdatesButton.accesskey' ) ), ] diff --git a/tests/migrate/test_merge.py b/tests/migrate/test_merge.py index 4da4af44..cdf9ed58 100644 --- a/tests/migrate/test_merge.py +++ b/tests/migrate/test_merge.py @@ -17,6 +17,8 @@ class MockContext(unittest.TestCase): def get_source(self, path, key): + # Ignore path (test.properties) and get translations from + # self.ab_cd_legacy defined in setUp. return self.ab_cd_legacy.get(key, None).val @@ -59,7 +61,7 @@ def setUp(self): self.transforms = [ FTL.Message( FTL.Identifier('title'), - value=COPY(None, 'aboutDownloads.title') + value=COPY('test.properties', 'aboutDownloads.title') ), FTL.Message( FTL.Identifier('about'), @@ -72,13 +74,13 @@ def setUp(self): attributes=[ FTL.Attribute( FTL.Identifier('label'), - COPY(None, 'aboutDownloads.open') + COPY('test.properties', 'aboutDownloads.open') ), ] ), FTL.Message( FTL.Identifier('download-state-downloading'), - value=COPY(None, 'downloadState.downloading') + value=COPY('test.properties', 'downloadState.downloading') ) ] @@ -169,20 +171,20 @@ def setUp(self): self.transforms = [ FTL.Message( FTL.Identifier('title'), - value=COPY(None, 'aboutDownloads.title') + value=COPY('test.properties', 'aboutDownloads.title') ), FTL.Message( FTL.Identifier('open-menuitem'), attributes=[ FTL.Attribute( FTL.Identifier('label'), - COPY(None, 'aboutDownloads.open') + COPY('test.properties', 'aboutDownloads.open') ), ] ), FTL.Message( FTL.Identifier('download-state-downloading'), - value=COPY(None, 'downloadState.downloading') + value=COPY('test.properties', 'downloadState.downloading') ) ] @@ -282,11 +284,11 @@ def setUp(self): self.transforms = [ FTL.Message( FTL.Identifier('title'), - value=COPY(None, 'aboutDownloads.title') + value=COPY('test.properties', 'aboutDownloads.title') ), FTL.Message( FTL.Identifier('download-state-downloading'), - value=COPY(None, 'downloadState.downloading') + value=COPY('test.properties', 'downloadState.downloading') ) ] diff --git a/tests/migrate/test_plural.py b/tests/migrate/test_plural.py index 095f232d..0fb21726 100644 --- a/tests/migrate/test_plural.py +++ b/tests/migrate/test_plural.py @@ -19,6 +19,8 @@ class MockContext(unittest.TestCase): plural_categories = ('one', 'other') def get_source(self, path, key): + # Ignore path (test.properties) and get translations from self.strings + # defined in setUp. return self.strings.get(key, None).val @@ -32,7 +34,7 @@ def setUp(self): self.message = FTL.Message( FTL.Identifier('delete-all'), value=PLURALS( - self.strings, + 'test.properties', 'deleteAll', EXTERNAL_ARGUMENT('num') ) @@ -86,7 +88,7 @@ def setUp(self): self.message = FTL.Message( FTL.Identifier('delete-all'), value=PLURALS( - self.strings, + 'test.properties', 'deleteAll', EXTERNAL_ARGUMENT('num') ) @@ -116,7 +118,7 @@ def test_plural_replace(self): msg = FTL.Message( FTL.Identifier('delete-all'), value=PLURALS( - self.strings, + 'test.properties', 'deleteAll', EXTERNAL_ARGUMENT('num'), lambda text: REPLACE_IN_TEXT( diff --git a/tests/migrate/test_replace.py b/tests/migrate/test_replace.py index 1157daf4..65576d00 100644 --- a/tests/migrate/test_replace.py +++ b/tests/migrate/test_replace.py @@ -16,6 +16,8 @@ class MockContext(unittest.TestCase): def get_source(self, path, key): + # Ignore path (test.properties) and get translations from self.strings + # defined in setUp. return self.strings.get(key, None).val @@ -33,7 +35,7 @@ def test_replace_one(self): msg = FTL.Message( FTL.Identifier(u'hello'), value=REPLACE( - self.strings, + 'test.properties', 'hello', { '#1': EXTERNAL_ARGUMENT('username') @@ -52,7 +54,7 @@ def test_replace_two(self): msg = FTL.Message( FTL.Identifier(u'welcome'), value=REPLACE( - self.strings, + 'test.properties', 'welcome', { '#1': EXTERNAL_ARGUMENT('username'), @@ -72,7 +74,7 @@ def test_replace_too_many(self): msg = FTL.Message( FTL.Identifier(u'welcome'), value=REPLACE( - self.strings, + 'test.properties', 'welcome', { '#1': EXTERNAL_ARGUMENT('username'), @@ -93,7 +95,7 @@ def test_replace_too_few(self): msg = FTL.Message( FTL.Identifier(u'welcome'), value=REPLACE( - self.strings, + 'test.properties', 'welcome', { '#1': EXTERNAL_ARGUMENT('username') @@ -112,7 +114,7 @@ def test_replace_first(self): msg = FTL.Message( FTL.Identifier(u'first'), value=REPLACE( - self.strings, + 'test.properties', 'first', { '#1': EXTERNAL_ARGUMENT('foo') @@ -131,7 +133,7 @@ def test_replace_last(self): msg = FTL.Message( FTL.Identifier(u'last'), value=REPLACE( - self.strings, + 'test.properties', 'last', { '#1': EXTERNAL_ARGUMENT('bar') diff --git a/tests/migrate/test_source.py b/tests/migrate/test_source.py new file mode 100644 index 00000000..1da96ac4 --- /dev/null +++ b/tests/migrate/test_source.py @@ -0,0 +1,51 @@ +# coding=utf8 +from __future__ import unicode_literals + +import unittest + +import fluent.syntax.ast as FTL + +from fluent.migrate.errors import NotSupportedError +from fluent.migrate.transforms import Source, COPY, PLURALS, REPLACE +from fluent.migrate.helpers import EXTERNAL_ARGUMENT + + +class TestNotSupportedError(unittest.TestCase): + def test_source(self): + pattern = ('Migrating translations from Fluent files is not supported') + with self.assertRaisesRegexp(NotSupportedError, pattern): + Source('test.ftl', 'foo') + + def test_copy(self): + pattern = ('Migrating translations from Fluent files is not supported') + with self.assertRaisesRegexp(NotSupportedError, pattern): + FTL.Message( + FTL.Identifier('foo'), + value=COPY('test.ftl', 'foo') + ) + + def test_plurals(self): + pattern = ('Migrating translations from Fluent files is not supported') + with self.assertRaisesRegexp(NotSupportedError, pattern): + FTL.Message( + FTL.Identifier('delete-all'), + value=PLURALS( + 'test.ftl', + 'deleteAll', + EXTERNAL_ARGUMENT('num') + ) + ) + + def test_replace(self): + pattern = ('Migrating translations from Fluent files is not supported') + with self.assertRaisesRegexp(NotSupportedError, pattern): + FTL.Message( + FTL.Identifier(u'hello'), + value=REPLACE( + 'test.ftl', + 'hello', + { + '#1': EXTERNAL_ARGUMENT('username') + } + ) + ) diff --git a/tools/migrate/examples/about_dialog.py b/tools/migrate/examples/about_dialog.py index 84dae067..bfd9528b 100644 --- a/tools/migrate/examples/about_dialog.py +++ b/tools/migrate/examples/about_dialog.py @@ -10,7 +10,7 @@ def migrate(ctx): """Migrate about:dialog, part {index}""" ctx.add_reference('browser/about_dialog.ftl', realpath='about_dialog.ftl') - ctx.add_localization('browser/chrome/browser/aboutDialog.dtd') + ctx.maybe_add_localization('browser/chrome/browser/aboutDialog.dtd') ctx.add_transforms('browser/about_dialog.ftl', [ FTL.Message( diff --git a/tools/migrate/examples/about_downloads.py b/tools/migrate/examples/about_downloads.py index 4d8ba1b2..887c4f22 100644 --- a/tools/migrate/examples/about_downloads.py +++ b/tools/migrate/examples/about_downloads.py @@ -11,8 +11,10 @@ def migrate(ctx): 'mobile/about_downloads.ftl', realpath='about_downloads.ftl' ) - ctx.add_localization('mobile/android/chrome/aboutDownloads.dtd') - ctx.add_localization('mobile/android/chrome/aboutDownloads.properties') + ctx.maybe_add_localization( + 'mobile/android/chrome/aboutDownloads.dtd') + ctx.maybe_add_localization( + 'mobile/android/chrome/aboutDownloads.properties') ctx.add_transforms('mobile/about_downloads.ftl', [ FTL.Message( diff --git a/tools/migrate/examples/bug_1291693.py b/tools/migrate/examples/bug_1291693.py index eebce64d..9200ee7c 100644 --- a/tools/migrate/examples/bug_1291693.py +++ b/tools/migrate/examples/bug_1291693.py @@ -20,10 +20,10 @@ def migrate(ctx): realpath='brand.ftl' ) - ctx.add_localization('browser/chrome/browser/browser.dtd') - ctx.add_localization('browser/chrome/browser/browser.properties') - ctx.add_localization('browser/branding/official/brand.dtd') - ctx.add_localization('browser/branding/official/brand.properties') + ctx.maybe_add_localization('browser/chrome/browser/browser.dtd') + ctx.maybe_add_localization('browser/chrome/browser/browser.properties') + ctx.maybe_add_localization('browser/branding/official/brand.dtd') + ctx.maybe_add_localization('browser/branding/official/brand.properties') ctx.add_transforms('browser/menubar.ftl', [ FTL.Message( diff --git a/tools/migrate/migrate-l10n.py b/tools/migrate/migrate-l10n.py index 6420991a..d8cf5cb1 100755 --- a/tools/migrate/migrate-l10n.py +++ b/tools/migrate/migrate-l10n.py @@ -2,14 +2,18 @@ # coding=utf8 import os +import sys import json +import logging import argparse import importlib import hglib from hglib.util import b -from fluent.migrate import MergeContext, convert_blame_to_changesets +from fluent.migrate import ( + MergeContext, MigrationError, convert_blame_to_changesets +) from blame import Blame @@ -25,18 +29,20 @@ def main(lang, reference_dir, localization_dir, blame, migrations, dry_run): # For each migration create a new context. ctx = MergeContext(lang, reference_dir, localization_dir) - # Add the migration spec. - migration.migrate(ctx) + try: + # Add the migration spec. + migration.migrate(ctx) + except MigrationError as err: + sys.exit(err.message) # Keep track of how many changesets we're committing. index = 0 for changeset in changesets: - # Run the migration. + # Run the migration for the changeset. snapshot = ctx.serialize_changeset(changeset['changes']) - # The current changeset didn't touch any of the translations - # affected by the migration. + # Did it change any files? if not snapshot: continue @@ -95,6 +101,9 @@ def main(lang, reference_dir, localization_dir, blame, migrations, dry_run): ) parser.set_defaults(dry_run=False) + logger = logging.getLogger('migrate') + logger.setLevel(logging.INFO) + args = parser.parse_args() if args.blame: From 9de0f5d4cfe80e7f58b2c212512e7bc664fbae54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sta=C5=9B=20Ma=C5=82olepszy?= Date: Mon, 30 Oct 2017 10:21:50 +0100 Subject: [PATCH 2/2] Bug 1321279 - Part 2 - Specify reference file in add_transforms. Remove MergeContext.add_reference in favor of an additional argument to MergeContext.add_transforms. This removes the redundancy of speficying the destination path twice. --- fluent/migrate/context.py | 37 +++++++++++---------- tests/migrate/test_context.py | 24 ++++++------- tests/migrate/test_context_real_examples.py | 6 ++-- tools/migrate/examples/about_dialog.py | 3 +- tools/migrate/examples/about_downloads.py | 6 +--- tools/migrate/examples/bug_1291693.py | 19 ++--------- 6 files changed, 36 insertions(+), 59 deletions(-) diff --git a/fluent/migrate/context.py b/fluent/migrate/context.py index 8c800589..974f2cbe 100644 --- a/fluent/migrate/context.py +++ b/fluent/migrate/context.py @@ -2,6 +2,7 @@ from __future__ import unicode_literals import os +import sys import codecs import logging @@ -109,22 +110,6 @@ def read_legacy_resource(self, path): # Transform the parsed result which is an iterator into a dict. return {entity.key: entity.val for entity in parser} - def add_reference(self, path, realpath=None): - """Add an FTL AST to this context's reference resources.""" - fullpath = os.path.join(self.reference_dir, realpath or path) - try: - ast = self.read_ftl_resource(fullpath) - except IOError as err: - error_message = 'Missing reference file: {}'.format(fullpath) - logging.getLogger('migrate').error(error_message) - raise UnreadableReferenceError(error_message) - except UnicodeDecodeError as err: - error_message = 'Error reading file {}: {}'.format(fullpath, err) - logging.getLogger('migrate').error(error_message) - raise UnreadableReferenceError(error_message) - else: - self.reference_resources[path] = ast - def maybe_add_localization(self, path): """Add a localization resource to migrate translations from. @@ -150,8 +135,8 @@ def maybe_add_localization(self, path): else: self.localization_resources[path] = collection - def add_transforms(self, path, transforms): - """Define transforms for path. + def add_transforms(self, path, reference, transforms): + """Define transforms for path using reference as template. Each transform is an extended FTL node with `Transform` nodes as some values. Transforms are stored in their lazy AST form until @@ -166,6 +151,22 @@ def get_sources(acc, cur): acc.add((cur.path, cur.key)) return acc + refpath = os.path.join(self.reference_dir, reference) + try: + ast = self.read_ftl_resource(refpath) + except IOError as err: + error_message = 'Missing reference file: {}'.format(refpath) + logging.getLogger('migrate').error(error_message) + raise UnreadableReferenceError(error_message) + except UnicodeDecodeError as err: + error_message = 'Error reading file {}: {}'.format(refpath, err) + logging.getLogger('migrate').error(error_message) + raise UnreadableReferenceError(error_message) + else: + # The reference file will be used by the merge function as + # a template for serializing the merge results. + self.reference_resources[path] = ast + for node in transforms: # Scan `node` for `Source` nodes and collect the information they # store into a set of dependencies. diff --git a/tests/migrate/test_context.py b/tests/migrate/test_context.py index 0016034e..4b7eef32 100644 --- a/tests/migrate/test_context.py +++ b/tests/migrate/test_context.py @@ -26,7 +26,6 @@ def setUp(self): localization_dir=here('fixtures/pl') ) - self.ctx.add_reference('aboutDownloads.ftl') try: self.ctx.maybe_add_localization('aboutDownloads.dtd') self.ctx.maybe_add_localization('aboutDownloads.properties') @@ -34,7 +33,7 @@ def setUp(self): self.skipTest('compare-locales required') def test_hardcoded_node(self): - self.ctx.add_transforms('aboutDownloads.ftl', [ + self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [ FTL.Message( id=FTL.Identifier('about'), value=FTL.Pattern([ @@ -59,7 +58,7 @@ def test_hardcoded_node(self): ) def test_merge_single_message(self): - self.ctx.add_transforms('aboutDownloads.ftl', [ + self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [ FTL.Message( id=FTL.Identifier('title'), value=COPY( @@ -85,7 +84,7 @@ def test_merge_single_message(self): ) def test_merge_one_changeset(self): - self.ctx.add_transforms('aboutDownloads.ftl', [ + self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [ FTL.Message( id=FTL.Identifier('title'), value=COPY( @@ -124,7 +123,7 @@ def test_merge_one_changeset(self): ) def test_merge_two_changesets(self): - self.ctx.add_transforms('aboutDownloads.ftl', [ + self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [ FTL.Message( id=FTL.Identifier('title'), value=COPY( @@ -177,7 +176,7 @@ def test_merge_two_changesets(self): self.assertDictEqual(merged_b, expected_b) def test_serialize_changeset(self): - self.ctx.add_transforms('aboutDownloads.ftl', [ + self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [ FTL.Message( id=FTL.Identifier('title'), value=COPY( @@ -247,7 +246,7 @@ def tearDown(self): def test_missing_reference_file(self): with self.assertRaises(UnreadableReferenceError): - self.ctx.add_reference('missing.ftl') + self.ctx.add_transforms('some.ftl', 'missing.ftl', []) class TestIncompleteLocalization(unittest.TestCase): @@ -261,13 +260,12 @@ def setUp(self): localization_dir=here('fixtures/pl') ) - self.ctx.add_reference('toolbar.ftl') try: self.ctx.maybe_add_localization('browser.dtd') except RuntimeError: self.skipTest('compare-locales required') - self.ctx.add_transforms('toolbar.ftl', [ + self.ctx.add_transforms('toolbar.ftl', 'toolbar.ftl', [ FTL.Message( id=FTL.Identifier('urlbar-textbox'), attributes=[ @@ -323,7 +321,6 @@ def setUp(self): localization_dir=here('fixtures/pl') ) - self.ctx.add_reference('privacy.ftl') try: self.ctx.maybe_add_localization('privacy.dtd') except RuntimeError: @@ -334,7 +331,7 @@ def tearDown(self): logging.disable(logging.NOTSET) def test_existing_target_ftl_missing_string(self): - self.ctx.add_transforms('privacy.ftl', [ + self.ctx.add_transforms('privacy.ftl', 'privacy.ftl', [ FTL.Message( id=FTL.Identifier('dnt-learn-more'), value=COPY( @@ -362,7 +359,7 @@ def test_existing_target_ftl_missing_string(self): ) def test_existing_target_ftl_existing_string(self): - self.ctx.add_transforms('privacy.ftl', [ + self.ctx.add_transforms('privacy.ftl', 'privacy.ftl', [ FTL.Message( id=FTL.Identifier('dnt-description'), value=COPY( @@ -401,7 +398,7 @@ def test_existing_target_ftl_existing_string(self): ) def test_existing_target_ftl_with_all_messages(self): - self.ctx.add_transforms('privacy.ftl', [ + self.ctx.add_transforms('privacy.ftl', 'privacy.ftl', [ FTL.Message( id=FTL.Identifier('dnt-description'), value=COPY( @@ -429,5 +426,4 @@ def test_add_ftl(self): localization_dir=here('fixtures/pl') ) - ctx.add_reference('privacy.ftl') ctx.maybe_add_localization('privacy.ftl') diff --git a/tests/migrate/test_context_real_examples.py b/tests/migrate/test_context_real_examples.py index 06bada70..e62f47ed 100644 --- a/tests/migrate/test_context_real_examples.py +++ b/tests/migrate/test_context_real_examples.py @@ -27,14 +27,13 @@ def setUp(self): localization_dir=here('fixtures/pl') ) - self.ctx.add_reference('aboutDownloads.ftl') try: self.ctx.maybe_add_localization('aboutDownloads.dtd') self.ctx.maybe_add_localization('aboutDownloads.properties') except RuntimeError: self.skipTest('compare-locales required') - self.ctx.add_transforms('aboutDownloads.ftl', [ + self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [ FTL.Message( id=FTL.Identifier('title'), value=COPY( @@ -289,12 +288,11 @@ def setUp(self): ) try: - self.ctx.add_reference('aboutDialog.ftl') self.ctx.maybe_add_localization('aboutDialog.dtd') except RuntimeError: self.skipTest('compare-locales required') - self.ctx.add_transforms('aboutDialog.ftl', [ + self.ctx.add_transforms('aboutDialog.ftl', 'aboutDialog.ftl', [ FTL.Message( id=FTL.Identifier('update-failed'), value=CONCAT( diff --git a/tools/migrate/examples/about_dialog.py b/tools/migrate/examples/about_dialog.py index bfd9528b..17ed610e 100644 --- a/tools/migrate/examples/about_dialog.py +++ b/tools/migrate/examples/about_dialog.py @@ -9,10 +9,9 @@ def migrate(ctx): """Migrate about:dialog, part {index}""" - ctx.add_reference('browser/about_dialog.ftl', realpath='about_dialog.ftl') ctx.maybe_add_localization('browser/chrome/browser/aboutDialog.dtd') - ctx.add_transforms('browser/about_dialog.ftl', [ + ctx.add_transforms('browser/about_dialog.ftl', 'about_dialog.ftl', [ FTL.Message( id=FTL.Identifier('update-failed'), value=CONCAT( diff --git a/tools/migrate/examples/about_downloads.py b/tools/migrate/examples/about_downloads.py index 887c4f22..35cd2beb 100644 --- a/tools/migrate/examples/about_downloads.py +++ b/tools/migrate/examples/about_downloads.py @@ -7,16 +7,12 @@ def migrate(ctx): """Migrate about:download in Firefox for Android, part {index}""" - ctx.add_reference( - 'mobile/about_downloads.ftl', - realpath='about_downloads.ftl' - ) ctx.maybe_add_localization( 'mobile/android/chrome/aboutDownloads.dtd') ctx.maybe_add_localization( 'mobile/android/chrome/aboutDownloads.properties') - ctx.add_transforms('mobile/about_downloads.ftl', [ + ctx.add_transforms('mobile/about_downloads.ftl', 'about_downloads.ftl', [ FTL.Message( id=FTL.Identifier('title'), value=COPY( diff --git a/tools/migrate/examples/bug_1291693.py b/tools/migrate/examples/bug_1291693.py index 9200ee7c..e745c9a5 100644 --- a/tools/migrate/examples/bug_1291693.py +++ b/tools/migrate/examples/bug_1291693.py @@ -7,25 +7,12 @@ def migrate(ctx): """Bug 1291693 - Migrate the menubar to FTL, part {index}""" - ctx.add_reference( - 'browser/menubar.ftl', - realpath='menubar.ftl' - ) - ctx.add_reference( - 'browser/toolbar.ftl', - realpath='toolbar.ftl' - ) - ctx.add_reference( - 'browser/branding/official/brand.ftl', - realpath='brand.ftl' - ) - ctx.maybe_add_localization('browser/chrome/browser/browser.dtd') ctx.maybe_add_localization('browser/chrome/browser/browser.properties') ctx.maybe_add_localization('browser/branding/official/brand.dtd') ctx.maybe_add_localization('browser/branding/official/brand.properties') - ctx.add_transforms('browser/menubar.ftl', [ + ctx.add_transforms('browser/menubar.ftl', 'menubar.ftl', [ FTL.Message( id=FTL.Identifier('file-menu'), attributes=[ @@ -1795,7 +1782,7 @@ def migrate(ctx): ), ]) - ctx.add_transforms('browser/toolbar.ftl', [ + ctx.add_transforms('browser/toolbar.ftl', 'toolbar.ftl', [ FTL.Message( id=FTL.Identifier('urlbar-textbox'), attributes=[ @@ -1889,7 +1876,7 @@ def migrate(ctx): ), ]) - ctx.add_transforms('browser/branding/official/brand.ftl', [ + ctx.add_transforms('browser/branding/official/brand.ftl', 'brand.ftl', [ FTL.Message( id=FTL.Identifier('brand-shorter-name'), value=COPY(