Repository: projectfluent/python-fluent

Issue:
Remove MESSAGE_REFERENCE and EXTERNAL_ARGUMENT helpers
The [`helpers.py`](https://github.com/projectfluent/python-fluent/blob/b40264348efe0dbad0565678533c24c21a2c4b0e/fluent/migrate/helpers.py) file currently defines two pseudo-AST nodes which can be used in migration specs:  `MESSAGE_REFERENCE` and `EXTERNAL_ARGUMENT`. I'd like to discuss the general approach regarding such helpers.

On one hand the allow to write more concise AST in migrations. On the other, they introduce abstractions which need to be learned besides the AST itself in order to write and maintain migrations. This in turn results in situations when it's tempting to randomly try different combinations of the helpers to see if they fix an issue with the migration :)

AST being the source of truth, I think there's value in sticking to it exclusively, even at the cost of increased verbosity.

@flodolo, @Pike, @zbraniecki - What do you think?

I already see @zbraniecki not use the helpers in [bug 1424682](https://bugzilla.mozilla.org/show_bug.cgi?id=1424682) in some cases, like here:

```py
FTL.Message(
    id = FTL.Identifier('category-general'),
    attributes = [
        FTL.Attribute(
            FTL.Identifier('tooltiptext'),
            FTL.Pattern(
                elements = [
                    FTL.Placeable(
                        # This could read:
                        # expression=MESSAGE_REFERENCE('pane-general-title')
                        expression = FTL.MessageReference(
                            id = FTL.Identifier(
                                'pane-general-title'
                            )
                        )
                    )
                ]
            )
        )
    ]
)
```

This is also related to what the API of `REPLACE` and `CONCAT` should be. Should we only accept `Patterns` and `PatternElements` (i.e. `TextElements` and `Placeables`)? This would make the API closer to the AST. Or should we accept `Expressions` as well? Compare:

```py
# REPLACE converts the MessageReference expression returned by the helper to a Placeable.
FTL.Message(
    id = FTL.Identifier('help-button-label'),
    value = REPLACE(
        'browser/chrome/browser/preferences/preferences.dtd',
        'helpButton2.label',
        {
            '&brandShortName;': MESSAGE_REFERENCE('-brand-short-name')
        }
    )
)
```

```py
# REPLACE requires an explicit Placeable wrapping the MessageReference expression.
FTL.Message(
    id = FTL.Identifier('help-button-label'),
    value = REPLACE(
        'browser/chrome/browser/preferences/preferences.dtd',
        'helpButton2.label',
        {
            '&brandShortName;': FTL.Placeable(
                MESSAGE_REFERENCE('-brand-short-name')
            )
        }
    )
)
```

```py
# REPLACE requires an explicit Placeable and an explicit MessageReference.
FTL.Message(
    id = FTL.Identifier('help-button-label'),
    value = REPLACE(
        'browser/chrome/browser/preferences/preferences.dtd',
        'helpButton2.label',
        {
            '&brandShortName;': FTL.Placeable(
                FTL.MessageReference(
                    FTL.Identifier('-brand-short-name')
                )
            )
        }
    )
)
```

Discussion:
stasm: @zbraniecki This doesn't change the way you've been using `fluent.migrate` so far. Are you OK with these changes?

Patch:
diff --git a/fluent/migrate/__init__.py b/fluent/migrate/__init__.py
index 3582468..93c8ab9 100644
--- a/fluent/migrate/__init__.py
+++ b/fluent/migrate/__init__.py
@@ -1,13 +1,5 @@
 # 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
-)
-from .helpers import (                                 # noqa: F401
-    EXTERNAL_ARGUMENT, MESSAGE_REFERENCE
+    CONCAT, COPY, PLURALS, REPLACE, REPLACE_IN_TEXT
 )
-from .changesets import convert_blame_to_changesets    # noqa: F401
diff --git a/fluent/migrate/context.py b/fluent/migrate/context.py
index 7d6a8a4..b1fc94c 100644
--- a/fluent/migrate/context.py
+++ b/fluent/migrate/context.py
@@ -285,7 +285,15 @@ class MergeContext(object):
 
         The input data must be configured earlier using the `add_*` methods.
         if given, `changeset` must be a set of (path, key) tuples describing
-        which legacy translations are to be merged.
+        which legacy translations are to be merged. If `changeset` is None,
+        all legacy translations will be allowed to be migrated in a single
+        changeset.
+
+        The inner `in_changeset` function is used to determine if a message
+        should be migrated for the given changeset. It compares the legacy
+        dependencies of the transform defined for the message with legacy
+        translations available in the changeset. If all dependencies are
+        present, the message will be migrated.
 
         Given `changeset`, return a dict whose keys are resource paths and
         values are `FTL.Resource` instances.  The values will also be used to
@@ -306,10 +314,18 @@ class MergeContext(object):
             transforms = self.transforms.get(path, [])
 
             def in_changeset(ident):
-                """Check if entity should be merged.
+                """Check if a message should be migrated.
+
+                A message will be migrated only if all of its dependencies
+                are present in the currently processed changeset.
+
+                If a transform defined for this message points to a missing
+                legacy translation, this message will not be merged. The
+                missing legacy dependency won't be present in the changeset.
 
-                If at least one dependency of the entity is in the current
-                set of changeset, merge it.
+                This also means that partially translated messages (e.g.
+                constructed from two legacy strings out of which only one is
+                avaiable) will never be migrated.
                 """
                 message_deps = self.dependencies.get((path, ident), None)
 
@@ -324,9 +340,11 @@ class MergeContext(object):
                 if len(message_deps) == 0:
                     return True
 
-                # If the intersection of the dependencies and the current
-                # changeset is non-empty, merge this message.
-                return message_deps & changeset
+                # Make sure all the dependencies are present in the current
+                # changeset. Partial migrations are not currently supported.
+                # See https://bugzilla.mozilla.org/show_bug.cgi?id=1321271
+                available_deps = message_deps & changeset
+                return message_deps == available_deps
 
             # Merge legacy translations with the existing ones using the
             # reference as a template.
diff --git a/fluent/migrate/transforms.py b/fluent/migrate/transforms.py
index b9390ac..1d4202e 100644
--- a/fluent/migrate/transforms.py
+++ b/fluent/migrate/transforms.py
@@ -277,43 +277,16 @@ class PLURALS(Source):
 
 
 class CONCAT(Transform):
-    """Concatenate elements of many patterns."""
+    """Create a new Pattern from Patterns, PatternElements and Expressions."""
 
-    def __init__(self, *patterns):
-        self.patterns = list(patterns)
+    def __init__(self, *elements, **kwargs):
+        # We want to support both passing elements as *elements in the
+        # migration specs and as elements=[]. The latter is used by
+        # FTL.BaseNode.traverse when it recreates the traversed node using its
+        # attributes as kwargs.
+        self.elements = list(kwargs.get('elements', elements))
 
     def __call__(self, ctx):
-        elements = self.flatten_elements(self.patterns)
+        elements = self.flatten_elements(self.elements)
         elements = self.prune_text_elements(elements)
         return FTL.Pattern(elements)
-
-    def traverse(self, fun):
-        def visit(value):
-            if isinstance(value, FTL.BaseNode):
-                return value.traverse(fun)
-            if isinstance(value, list):
-                return fun(map(visit, value))
-            else:
-                return fun(value)
-
-        node = self.__class__(
-            *[
-                visit(value) for value in self.patterns
-            ]
-        )
-
-        return fun(node)
-
-    def to_json(self):
-        def to_json(value):
-            if isinstance(value, FTL.BaseNode):
-                return value.to_json()
-            else:
-                return value
-
-        return {
-            'type': self.__class__.__name__,
-            'patterns': [
-                to_json(value) for value in self.patterns
-            ]
-        }
diff --git a/fluent/syntax/ast.py b/fluent/syntax/ast.py
index ff8b0ee..faccf08 100644
--- a/fluent/syntax/ast.py
+++ b/fluent/syntax/ast.py
@@ -66,12 +66,10 @@ class BaseNode(object):
             else:
                 return fun(value)
 
+        # Use all attributes found on the node as kwargs to the constructor.
+        kwargs = vars(self).items()
         node = self.__class__(
-            **{
-                name: visit(value)
-                for name, value in vars(self).items()
-            }
-        )
+            **{name: visit(value) for name, value in kwargs})
 
         return fun(node)
 
diff --git a/tools/migrate/examples/about_dialog.py b/tools/migrate/examples/about_dialog.py
index e45def1..7afd7d9 100644
--- a/tools/migrate/examples/about_dialog.py
+++ b/tools/migrate/examples/about_dialog.py
@@ -1,9 +1,8 @@
 # coding=utf8
 
 import fluent.syntax.ast as FTL
-from fluent.migrate import (
-    CONCAT, EXTERNAL_ARGUMENT, MESSAGE_REFERENCE, COPY, REPLACE
-)
+from fluent.migrate import CONCAT, COPY, REPLACE
+from fluent.migrate.helpers import EXTERNAL_ARGUMENT, MESSAGE_REFERENCE
 
 
 def migrate(ctx):
diff --git a/tools/migrate/examples/about_downloads.py b/tools/migrate/examples/about_downloads.py
index edfc823..a07d9b0 100644
--- a/tools/migrate/examples/about_downloads.py
+++ b/tools/migrate/examples/about_downloads.py
@@ -1,7 +1,8 @@
 # coding=utf8
 
 import fluent.syntax.ast as FTL
-from fluent.migrate import EXTERNAL_ARGUMENT, COPY, PLURALS, REPLACE_IN_TEXT
+from fluent.migrate import COPY, PLURALS, REPLACE_IN_TEXT
+from fluent.migrate.helpers import EXTERNAL_ARGUMENT
 
 
 def migrate(ctx):
diff --git a/tools/migrate/examples/bug_1291693.py b/tools/migrate/examples/bug_1291693.py
index b05327b..6fd27c4 100644
--- a/tools/migrate/examples/bug_1291693.py
+++ b/tools/migrate/examples/bug_1291693.py
@@ -1,7 +1,8 @@
 # coding=utf8
 
 import fluent.syntax.ast as FTL
-from fluent.migrate import MESSAGE_REFERENCE, COPY, REPLACE
+from fluent.migrate import COPY, REPLACE
+from fluent.migrate.helpers import MESSAGE_REFERENCE
 
 
 def migrate(ctx):
diff --git a/tools/migrate/migrate-l10n.py b/tools/migrate/migrate-l10n.py
index 8070d5f..b57d022 100755
--- a/tools/migrate/migrate-l10n.py
+++ b/tools/migrate/migrate-l10n.py
@@ -11,9 +11,9 @@ import importlib
 import hglib
 from hglib.util import b
 
-from fluent.migrate import (
-    MergeContext, MigrationError, convert_blame_to_changesets
-)
+from fluent.migrate.context import MergeContext
+from fluent.migrate.errors import MigrationError
+from fluent.migrate.changesets import convert_blame_to_changesets
 from blame import Blame
 
 


Test patch:
diff --git a/tests/migrate/test_concat.py b/tests/migrate/test_concat.py
index 9cba145..6d2f35e 100644
--- a/tests/migrate/test_concat.py
+++ b/tests/migrate/test_concat.py
@@ -157,7 +157,7 @@ class TestConcatInterpolate(MockContext):
             <!ENTITY channel.description.end    " channel.">
         ''')
 
-    def test_concat_replace(self):
+    def test_concat_placeable(self):
         msg = FTL.Message(
             FTL.Identifier('channel-desc'),
             value=CONCAT(
@@ -174,6 +174,23 @@ class TestConcatInterpolate(MockContext):
             ''')
         )
 
+    def test_concat_expression(self):
+        msg = FTL.Message(
+            FTL.Identifier('channel-desc'),
+            value=CONCAT(
+                COPY('test.properties', 'channel.description.start'),
+                EXTERNAL_ARGUMENT('channelname'),
+                COPY('test.properties', 'channel.description.end'),
+            )
+        )
+
+        self.assertEqual(
+            evaluate(self, msg).to_json(),
+            ftl_message_to_json('''
+                channel-desc = You are on the { $channelname } channel.
+            ''')
+        )
+
 
 @unittest.skipUnless(DTDParser, 'compare-locales required')
 class TestConcatReplace(MockContext):
diff --git a/tests/migrate/test_context.py b/tests/migrate/test_context.py
index 81d55af..56f0f50 100644
--- a/tests/migrate/test_context.py
+++ b/tests/migrate/test_context.py
@@ -16,7 +16,7 @@ from fluent.migrate.errors import (
     EmptyLocalizationError, 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
+from fluent.migrate.transforms import CONCAT, COPY
 
 
 def here(*parts):
@@ -251,7 +251,7 @@ class TestIncompleteReference(unittest.TestCase):
 
 
 @unittest.skipUnless(compare_locales, 'compare-locales requried')
-class TestEmptyLocalization(unittest.TestCase):
+class TestMissingLocalizationFiles(unittest.TestCase):
     def setUp(self):
         # Silence all logging.
         logging.disable(logging.CRITICAL)
@@ -266,7 +266,40 @@ class TestEmptyLocalization(unittest.TestCase):
         # Resume logging.
         logging.disable(logging.NOTSET)
 
-    def test_all_localization_missing(self):
+    def test_missing_file(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
+            FTL.Message(
+                id=FTL.Identifier('title'),
+                value=COPY(
+                    'aboutDownloads.dtd',
+                    'aboutDownloads.title'
+                )
+            ),
+            FTL.Message(
+                id=FTL.Identifier('header'),
+                value=COPY(
+                    'missing.dtd',
+                    'missing'
+                )
+            ),
+        ])
+
+        expected = {
+            'aboutDownloads.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/.
+
+        title = Pobrane pliki
+            ''')
+        }
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            expected
+        )
+
+    def test_all_files_missing(self):
         pattern = ('No localization files were found')
         with self.assertRaisesRegexp(EmptyLocalizationError, pattern):
             self.ctx.add_transforms('existing.ftl', 'existing.ftl', [
@@ -281,7 +314,9 @@ class TestEmptyLocalization(unittest.TestCase):
 
 
 @unittest.skipUnless(compare_locales, 'compare-locales requried')
-class TestIncompleteLocalization(unittest.TestCase):
+class TestMissingLocalizationStrings(unittest.TestCase):
+    maxDiff = None
+
     def setUp(self):
         # Silence all logging.
         logging.disable(logging.CRITICAL)
@@ -296,20 +331,116 @@ class TestIncompleteLocalization(unittest.TestCase):
         # Resume logging.
         logging.disable(logging.NOTSET)
 
-    def test_missing_localization_file(self):
-        self.ctx.add_transforms('existing.ftl', 'existing.ftl', [
+    def test_missing_string_in_simple_value(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
             FTL.Message(
-                id=FTL.Identifier('foo'),
+                id=FTL.Identifier('title'),
                 value=COPY(
-                    'existing.dtd',
-                    'foo'
+                    'aboutDownloads.dtd',
+                    'missing'
                 )
             ),
+        ])
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            {}
+        )
+
+    def test_missing_string_in_only_variant(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
             FTL.Message(
-                id=FTL.Identifier('bar'),
-                value=COPY(
-                    'missing.dtd',
-                    'bar'
+                id=FTL.Identifier('title'),
+                value=CONCAT(
+                    FTL.SelectExpression(
+                        expression=FTL.CallExpression(
+                            callee=FTL.Identifier('PLATFORM')
+                        ),
+                        variants=[
+                            FTL.Variant(
+                                key=FTL.VariantName('other'),
+                                default=True,
+                                value=COPY(
+                                    'aboutDownloads.dtd',
+                                    'missing'
+                                )
+                            ),
+                        ]
+                    ),
+                )
+            ),
+        ])
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            {}
+        )
+
+    def test_missing_string_in_all_variants(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
+            FTL.Message(
+                id=FTL.Identifier('title'),
+                value=CONCAT(
+                    FTL.SelectExpression(
+                        expression=FTL.CallExpression(
+                            callee=FTL.Identifier('PLATFORM')
+                        ),
+                        variants=[
+                            FTL.Variant(
+                                key=FTL.VariantName('windows'),
+                                default=False,
+                                value=COPY(
+                                    'aboutDownloads.dtd',
+                                    'missing.windows'
+                                )
+                            ),
+                            FTL.Variant(
+                                key=FTL.VariantName('other'),
+                                default=True,
+                                value=COPY(
+                                    'aboutDownloads.dtd',
+                                    'missing.other'
+                                )
+                            ),
+                        ]
+                    ),
+                )
+            ),
+        ])
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            {}
+        )
+
+    def test_missing_string_in_one_of_variants(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
+            FTL.Message(
+                id=FTL.Identifier('title'),
+                value=CONCAT(
+                    FTL.SelectExpression(
+                        expression=FTL.CallExpression(
+                            callee=FTL.Identifier('PLATFORM')
+                        ),
+                        variants=[
+                            FTL.Variant(
+                                key=FTL.VariantName('windows'),
+                                default=False,
+                                value=COPY(
+                                    'aboutDownloads.dtd',
+                                    'aboutDownloads.title'
+                                )
+                            ),
+                            FTL.Variant(
+                                key=FTL.VariantName('other'),
+                                default=True,
+                                value=COPY(
+                                    'aboutDownloads.dtd',
+                                    'missing.other'
+                                )
+                            ),
+                        ]
+                    ),
                 )
             ),
         ])
@@ -319,6 +450,83 @@ class TestIncompleteLocalization(unittest.TestCase):
             {}
         )
 
+    def test_missing_string_in_only_attribute(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
+            FTL.Message(
+                id=FTL.Identifier('title'),
+                attributes=[
+                    FTL.Attribute(
+                        FTL.Identifier('one'),
+                        COPY(
+                            'aboutDownloads.dtd',
+                            'missing'
+                        )
+                    ),
+                ]
+            ),
+        ])
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            {}
+        )
+
+    def test_missing_string_in_all_attributes(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
+            FTL.Message(
+                id=FTL.Identifier('title'),
+                attributes=[
+                    FTL.Attribute(
+                        FTL.Identifier('one'),
+                        COPY(
+                            'aboutDownloads.dtd',
+                            'missing.one'
+                        )
+                    ),
+                    FTL.Attribute(
+                        FTL.Identifier('two'),
+                        COPY(
+                            'aboutDownloads.dtd',
+                            'missing.two'
+                        )
+                    ),
+                ]
+            ),
+        ])
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            {}
+        )
+
+    def test_missing_string_in_one_of_attributes(self):
+        self.ctx.add_transforms('aboutDownloads.ftl', 'aboutDownloads.ftl', [
+            FTL.Message(
+                id=FTL.Identifier('title'),
+                attributes=[
+                    FTL.Attribute(
+                        FTL.Identifier('title'),
+                        COPY(
+                            'aboutDownloads.dtd',
+                            'aboutDownloads.title'
+                        )
+                    ),
+                    FTL.Attribute(
+                        FTL.Identifier('missing'),
+                        COPY(
+                            'aboutDownloads.dtd',
+                            'missing'
+                        )
+                    ),
+                ]
+            ),
+        ])
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset()),
+            {}
+        )
+
 
 @unittest.skipUnless(compare_locales, 'compare-locales requried')
 class TestExistingTarget(unittest.TestCase):
@@ -360,7 +568,6 @@ class TestExistingTarget(unittest.TestCase):
             ''')
         }
 
-        self.maxDiff = None
         self.assertDictEqual(
             to_json(self.ctx.merge_changeset()),
             expected
@@ -399,7 +606,6 @@ class TestExistingTarget(unittest.TestCase):
             ''')
         }
 
-        self.maxDiff = None
         self.assertDictEqual(
             to_json(self.ctx.merge_changeset()),
             expected
diff --git a/tests/migrate/test_context_real_examples.py b/tests/migrate/test_context_real_examples.py
index 482dbc8..1586826 100644
--- a/tests/migrate/test_context_real_examples.py
+++ b/tests/migrate/test_context_real_examples.py
@@ -281,6 +281,8 @@ class TestMergeAboutDownloads(unittest.TestCase):
 
 @unittest.skipUnless(compare_locales, 'compare-locales requried')
 class TestMergeAboutDialog(unittest.TestCase):
+    maxDiff = None
+
     def setUp(self):
         self.ctx = MergeContext(
             lang='pl',
@@ -363,6 +365,8 @@ class TestMergeAboutDialog(unittest.TestCase):
     def test_merge_context_some_messages(self):
         changeset = {
             ('aboutDialog.dtd', 'update.failed.start'),
+            ('aboutDialog.dtd', 'update.failed.linkText'),
+            ('aboutDialog.dtd', 'update.failed.end'),
         }
 
         expected = {
@@ -379,3 +383,14 @@ class TestMergeAboutDialog(unittest.TestCase):
             to_json(self.ctx.merge_changeset(changeset)),
             expected
         )
+
+    def test_merge_context_too_few_messages(self):
+        changeset = {
+            ('aboutDialog.dtd', 'update.failed.start'),
+            ('aboutDialog.dtd', 'update.failed.linkText'),
+        }
+
+        self.assertDictEqual(
+            to_json(self.ctx.merge_changeset(changeset)),
+            {}
+        )
diff --git a/tests/migrate/test_merge.py b/tests/migrate/test_merge.py
index a85fead..9178326 100644
--- a/tests/migrate/test_merge.py
+++ b/tests/migrate/test_merge.py
@@ -19,12 +19,17 @@ 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
+        translation = self.ab_cd_legacy.get(key, None)
+
+        if translation is not None:
+            return translation.val
 
 
 @unittest.skipUnless(PropertiesParser and DTDParser,
                      'compare-locales required')
 class TestMergeMessages(MockContext):
+    maxDiff = None
+
     def setUp(self):
         self.en_us_ftl = parse(FluentParser, ftl('''
             title  = Downloads
diff --git a/tests/migrate/test_util.py b/tests/migrate/test_util.py
index bc739fa..32ea42d 100644
--- a/tests/migrate/test_util.py
+++ b/tests/migrate/test_util.py
@@ -27,11 +27,11 @@ class TestTraverse(unittest.TestCase):
         result = node.traverse(lambda x: x)
 
         self.assertEqual(
-            result.value.patterns[0].key,
+            result.value.elements[0].key,
             'key1'
         )
         self.assertEqual(
-            result.value.patterns[1].key,
+            result.value.elements[1].key,
             'key2'
         )