Merge pull request #24 from stasm/1321279-read-ftl-when-migrating

 Bug 1321279 - Read target FTL files before migrations.

Author: stasm

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
 )

fluent/migrate/context.py

@@ -2,6 +2,7 @@
 from __future__ import unicode_literals
 
 import os
+import sys
 import codecs
 import logging
 
@@ -18,7 +19,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 +80,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 +99,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
 
@@ -105,52 +110,33 @@ 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:
-            logger = logging.getLogger('migrate')
-            logger.error(u'Missing reference file: {}'.format(path))
-            raise err
-        except UnicodeDecodeError as err:
-            logger = logging.getLogger('migrate')
-            logger.error(u'Error reading file {}: {}'.format(path, err))
-            raise err
-        else:
-            self.reference_resources[path] = ast
+    def maybe_add_localization(self, path):
+        """Add a localization resource to migrate translations from.
 
-    def add_localization(self, path):
-        """Add an existing localization resource.
+        Only legacy resources can be added as migration sources.  The resource
+        may be missing on disk.
 
-        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.
+        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.
+    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
@@ -165,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.
@@ -175,17 +177,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 +215,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 +256,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
 

fluent/migrate/errors.py

@@ -0,0 +1,10 @@
+class MigrationError(ValueError):
+    pass
+
+
+class NotSupportedError(MigrationError):
+    pass
+
+
+class UnreadableReferenceError(MigrationError):
+    pass

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):

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
 

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

tests/migrate/fixtures/pl/privacy.dtd

@@ -0,0 +1,7 @@
+<!-- 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/. -->
+
+<!ENTITY doNotTrack.description "Old Description in Polish">
+<!ENTITY doNotTrack.learnMore.label "Więcej informacji">
+<!ENTITY doNotTrack.always.label "Zawsze">

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

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('<a>'),
-                COPY(self.strings, 'update.failed.linkText'),
+                COPY('test.properties', 'update.failed.linkText'),
                 FTL.TextElement('</a>'),
-                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('<a>'),
                 REPLACE(
-                    self.strings,
+                    'test.properties',
                     'community.mozillaLink',
                     {
                         '&vendorShortName;': MESSAGE_REFERENCE(
@@ -206,11 +208,11 @@ def test_concat_replace(self):
                     }
                 ),
                 FTL.TextElement('</a>'),
-                COPY(self.strings, 'community.middle'),
+                COPY('test.properties', 'community.middle'),
                 FTL.TextElement('<a>'),
-                COPY(self.strings, 'community.creditsLink'),
+                COPY('test.properties', 'community.creditsLink'),
                 FTL.TextElement('</a>'),
-                COPY(self.strings, 'community.end')
+                COPY('test.properties', 'community.end')
             )
         )