Closed Bug 1581952 Opened 5 years ago Closed 2 years ago

Improve migration writing UX

Categories

(Localization Infrastructure and Tools :: Fluent Migration, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zbraniecki, Unassigned)

References

Details

Bug 1568133 gives us an example of a very hard to write migration code.

For example, this chunk:

            FTL.Message(
                id=FTL.Identifier('menu-quit'),
                attributes=[
                    FTL.Attribute(
                        FTL.Identifier('label'),
                        FTL.Pattern(
                            elements=[
                                FTL.Placeable(
                                    expression=FTL.SelectExpression(
                                        selector=FTL.FunctionReference(
                                            id=FTL.Identifier('PLATFORM'),
                                            arguments=FTL.CallArguments([], [])
                                        ),
                                        variants=[
                                            FTL.Variant(
                                                key=FTL.Identifier('windows'),
                                                default=False,
                                                value=REPLACE(
                                                    'browser/chrome/browser/browser.dtd',
                                                    'quitApplicationCmdWin2.label',
                                                    {
                                                        "&brandShorterName;": TERM_REFERENCE("brand-shorter-name"),
                                                    }
                                                )
                                            ),
                                            FTL.Variant(
                                                key=FTL.Identifier('macos'),
                                                default=False,
                                                value=REPLACE(
                                                    'browser/chrome/browser/browser.dtd',
                                                    'quitApplicationCmdMac2.label',
                                                    {
                                                        "&brandShorterName;": TERM_REFERENCE("brand-shorter-name"),
                                                    }
                                                )
                                            ),
                                            FTL.Variant(
                                                key=FTL.Identifier('other'),
                                                default=True,
                                                value=REPLACE(
                                                    'browser/chrome/browser/browser.dtd',
                                                    'quitApplicationCmd.label',
                                                    {
                                                        "&brandShorterName;": TERM_REFERENCE("brand-shorter-name"),
                                                    }
                                                )
                                            )
                                        ]
                                    )
                                )
                            ]
                        )
                    ),
                    FTL.Attribute(
                        FTL.Identifier('accesskey'),
                        FTL.Pattern(
                            elements=[
                                FTL.Placeable(
                                    expression=FTL.SelectExpression(
                                        selector=FTL.FunctionReference(
                                            id=FTL.Identifier('PLATFORM'),
                                            arguments=FTL.CallArguments([], [])
                                        ),
                                        variants=[
                                            FTL.Variant(
                                                key=FTL.Identifier('windows'),
                                                default=False,
                                                value=COPY(
                                                    'browser/chrome/browser/browser.dtd',
                                                    'quitApplicationCmdWin2.accesskey'
                                                )
                                            ),
                                            FTL.Variant(
                                                key=FTL.Identifier('macos'),
                                                default=False,
                                                value=FTL.Pattern(elements=[FTL.Placeable(
                                                    expression=FTL.StringLiteral(
                                                        ""
                                                    )
                                                )])
                                            ),
                                            FTL.Variant(
                                                key=FTL.Identifier('other'),
                                                default=True,
                                                value=COPY(
                                                    'browser/chrome/browser/browser.dtd',
                                                    'quitApplicationCmd.accesskey'
                                                )
                                            )
                                        ]
                                    )
                                )
                            ]
                        )
                    ),
                ]
            ),

would be way less prone to error and easier to write and review if it was encoded as:

menu-quit =
    .label =
        { PLATFORM() ->
            [windows] { REPLACE_BRANDS(base_path, "quitApplicationCmdWin2.label") }
            [macos] { REPLACE_BRANDS(base_path, "quitApplicationCmdMac2.label") }
           *[other] { REPLACE_BRANDS(base_path, "quitApplicationCmd.label") }
        }
    .accesskey =
        { PLATFORM() ->
            [windows] { COPY(base_path, "quitApplicationCmdWin2.accesskey") }
            [macos] { "" }
           *[other] { COPY(base_path, "quitApplicationCmd.accesskey") }
        }

Since brand replacement is the most common operation, we could provide a helper method, and the rest is just building up on top of the current templating.

Here's a mock of how the full migration script might look like:

# Any copyright is dedicated to the Public Domain.
# http://creativecommons.org/publicdomain/zero/1.0/

from __future__ import absolute_import
import fluent.syntax.ast as FTL
from fluent.migrate.helpers import transforms_from
from fluent.migrate import COPY, CONCAT, REPLACE
from fluent.migrate.helpers import MESSAGE_REFERENCE, TERM_REFERENCE, VARIABLE_REFERENCE


def migrate(ctx):
    """Bug 1568133 - Migrate remaining menubar from dtd to ftl, part {index}"""

    ctx.add_transforms(
        'browser/browser/menubar.ftl',
        'browser/browser/menubar.ftl',
        transforms_from(
"""
menu-application-services =
    .label = { COPY(base_path, "servicesMenuMac.label") }
menu-application-hide-this =
    .label = { REPLACE_BRANDS(base_path, "hideOtherAppsCmdMac.label") }
menu-application-hide-other =
    .label = { COPY(base_path, "hideOtherAppsCmdMac.label") }
menu-application-show-all =
    .label = { COPY(base_path, "showAllAppsCmdMac.label") }
menu-application-touch-bar =
    .label = { COPY(base_path, "touchBarCmdMac.label") }
menu-quit =
    .label =
        { PLATFORM() ->
            [windows] { REPLACE_BRANDS(browser_path, "quitApplicationCmdWin2.label") }
            [macos] { REPLACE_BRANDS(browser_path, "quitApplicationCmdMac2.label") }
           *[other] { REPLACE_BRANDS(browser_path, "quitApplicationCmd.label") }
        }
    .accesskey =
        { PLATFORM() ->
            [windows] { COPY(browser_path, "quitApplicationCmdWin2.accesskey") }
            [macos] { "" }
           *[other] { COPY(browser_path, "quitApplicationCmd.accesskey") }
        }
menu-quit-button =
    .label = { menu-quit.label }
    .tooltip =
        { PLATFORM() ->
            [windows] { REPLACE_BRANDS(browser_path, "quitApplicationCmdWin2.tooltip") }
           *[other] { "" }
        }
menu-about =
    .label = { REPLACE_BRANDS(base_path, "aboutProduct2.label")
    .accesskey = { COPY(base_path, "aboutProduct2.accesskey") }

""", base_path="browser/chrome/browser/baseMenuOverlay.dtd", browser_path="browser/chrome/browser/browser.dtd"))

See Also: → 1568133

Writing the linked migration was very painful and would have been even worse if Zibi wasn't sitting next to me. Adding some helpers for these common cases would go a long way.

Anything we could do to make this easier will pay off. Especially since as things currently stand these migrations will still be required even after initial migration to fluent (sinc revving a string in ftl requires writing a migration as well and devs are accustomed to not having this additional step with dtd/properties).

since revving a string in ftl requires writing a migration as well and devs are accustomed to not having this additional step with dtd/properties

This should only apply to refactors where value or attribute logic change (.label -> .value etc). That might be a thing right now, but surely won't be a lasting problem.

The regular changing of UI that requires new translations doesn't involve any new processes.

(In reply to Axel Hecht [:Pike] from comment #4)

since revving a string in ftl requires writing a migration as well and devs are accustomed to not having this additional step with dtd/properties

This should only apply to refactors where value or attribute logic change (.label -> .value etc). That might be a thing right now, but surely won't be a lasting problem.

The regular changing of UI that requires new translations doesn't involve any new processes.

Thanks for the clarification, that's good to know. When changing a string without attribute logic changes, should we continue to rev the ID or change only the string value?

(In reply to Brian Grinstead [:bgrins] from comment #5)

Thanks for the clarification, that's good to know. When changing a string without attribute logic changes, should we continue to rev the ID or change only the string value?

Change the ID. See also
https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_review.html

As we're using fluent.migrate in bedrock now, too, it's more clear that we shouldn't build gecko-specific transforms into fluent.migrate itself. Argueably, transforms.PLURALS is also a good example of transforms that should probably live in mozilla-central natively.

I just filed and started hacking around bug 1596726. Is that the right start to address the concerns here?

In particular, I'm envisioning that the gecko-specific subclass could watch out for

TermReference(id=Identifier(name='-brand*'), atttribute=None, arguments=None)

and generate a REPLACE item instead of the TermReference one. If it hooks itself up in Pattern, I guess.

As far as I can tell, this was solved by adding the transforms_from() helper.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.