Address Pike's feedback

Author: stasm

fluent/migrate/context.py

@@ -256,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/merge.py

@@ -12,7 +12,7 @@ def merge_resource(ctx, reference, current, transforms, in_changeset):
 
     Use the `reference` FTL AST as a template.  For each en-US string in the
     reference, first check for an existing translation in the current FTL
-    `localization` and return early if it's present; then if the string has
+    `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.
     """

tests/migrate/fixtures/pl/existing.ftl

@@ -397,6 +397,24 @@ def test_existing_target_ftl_existing_string(self):
             expected
         )
 
+    def test_existing_target_ftl_with_all_messages(self):
+        self.ctx.add_transforms('privacy.ftl', '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):

tests/migrate/test_context.py

@@ -9,21 +9,10 @@
 except ImportError:
     PropertiesParser = DTDParser = None
 
-from fluent.migrate.errors import NotSupportedError
 from fluent.migrate.util import parse, ftl_message_to_json
 from fluent.migrate.transforms import evaluate, COPY
 
 
-class TestNotSupportedError(unittest.TestCase):
-    def test_fluent(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')
-            )
-
-
 class MockContext(unittest.TestCase):
     def get_source(self, path, key):
         # Ignore path (test.properties) and get translations from self.strings

tests/migrate/test_copy.py

@@ -9,26 +9,11 @@
 except ImportError:
     PropertiesParser = None
 
-from fluent.migrate.errors import NotSupportedError
 from fluent.migrate.util import parse, ftl_message_to_json
 from fluent.migrate.helpers import EXTERNAL_ARGUMENT
 from fluent.migrate.transforms import evaluate, PLURALS, REPLACE_IN_TEXT
 
 
-class TestNotSupportedError(unittest.TestCase):
-    def test_fluent(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')
-                )
-            )
-
-
 class MockContext(unittest.TestCase):
     # Static categories corresponding to en-US.
     plural_categories = ('one', 'other')

tests/migrate/test_plural.py

@@ -9,28 +9,11 @@
 except ImportError:
     PropertiesParser = None
 
-from fluent.migrate.errors import NotSupportedError
 from fluent.migrate.util import parse, ftl_message_to_json
 from fluent.migrate.helpers import EXTERNAL_ARGUMENT
 from fluent.migrate.transforms import evaluate, REPLACE
 
 
-class TestNotSupportedError(unittest.TestCase):
-    def test_fluent(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')
-                    }
-                )
-            )
-
-
 class MockContext(unittest.TestCase):
     def get_source(self, path, key):
         # Ignore path (test.properties) and get translations from self.strings

tests/migrate/test_replace.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')
+                    }
+                )
+            )