Closed Bug 1467183 Opened 6 years ago Closed 6 years ago

Write l10n migration documentation

Categories

(Core :: Internationalization, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: zbraniecki, Assigned: flod)

Details

Attachments

(1 file)

One of the lessons learned during the Preferences post-mortem was that we should do better job at explaining ourselves around migrations.

Trying to capture devs frame of mind, the migration docs should explain:

 - what is the problem at hand (migrating to Fluent leaves behind work of 100+ l10n communities)
 - how migrations solve this problem
 - how migrations fit into a migration cycle (write a patch, attach migration, land together, we'll do migrations, when the migration script can be removed)
 - how to write a migration
   - easy case
   - common outliers: concatenation, merging two strings into compound message, introducing plurals, introducing -brand-name reference etc.
 - how test migration
 - who to ask for help
Assignee: nobody → francesco.lodolo
Priority: -- → P3
@zibi
Where do you think this doc should live? I think it would be long enough to need its own separate file in
https://firefox-source-docs.mozilla.org/intl/l10n/docs/l10n/index.html
Yup, new file in intl/l10n/docs :)
I feel like I've made good progress on the doc, and it's in good shape for review.

There's one point that I'm missing, and that's at least a link to a document explaining the AST at high level, but it doesn't look like such a document exists? I thought of linking to the PlayGround, and look at the AST there, but the amount of information is scary compared to what a developer needs to write migrations.

I might be also misusing some terminology, e.g. Transforms.

I've looked at the comments in some fluent-migration Python files (transforms.py in particular), and while I can write migration easily for most cases, I can't understand much of those comments (Patterns, Placeable, etc.) :)
Comment on attachment 8984195 [details]
Bug 1467183 - Add documentation for Fluent migrations

https://reviewboard.mozilla.org/r/249996/#review256312

This is a pretty good start, I found a couple of bugs, though.

The details of how migrations are applied might come in handy after the testing part, to give developers an idea that they're actually doing the real deal.

::: intl/l10n/docs/fluent_migrations.rst:22
(Diff revision 1)
> +to localize hundreds of strings from scratch.
> +
> +`Fluent Migration`_ is a Python library designed to solve this specific problem:
> +it allows to migrate legacy translations from `.dtd` and `.properties` files,
> +not only moving strings and transforming them as needed to adapt to the `FTL`
> +syntax, but also replicating in VCS the history of each string.

replicating "blame" for each string in VCS

::: intl/l10n/docs/fluent_migrations.rst:38
(Diff revision 1)
> +__ https://hg.mozilla.org/mozilla-central/file/default/python/l10n/fluent_migrations
> +
> +When a feature is migrated to Fluent, a migration recipe should be attached to
> +the same patch that adds new strings to `.ftl` files.
> +
> +Once the patch lands in mozilla-central, l10n-drivers will perform a series of

I'd move 38-49 to the very bottom, just for the curious. I think this is too much detail upfront.

::: intl/l10n/docs/fluent_migrations.rst:120
(Diff revision 1)
> +  def migrate(ctx):
> +      """Bug 1411707 - Migrate the findbar XBL binding to a Custom Element, part {index}."""
> +
> +      ctx.add_transforms(
> +          "toolkit/toolkit/main-window/findbar.ftl",
> +          "toolkit/toolkit/main-window/findbar.ftl",

I wouldn't mind if this was just `toolkit/main-window/findbar.ftl`. Documenting `bla/bla` seems to set a bad incentive.

::: intl/l10n/docs/fluent_migrations.rst:173
(Diff revision 1)
> +
> +
> +This method of writing migration recipes allows to take the original FTL
> +strings, and simply replace the value of each message with a :python:`COPY`
> +Transform. :python:`transforms_from` takes care of converting the FTL-like
> +syntax into an array of FTL messages, unfortunately it’s suitable only for

s/unfortunately/something else/

... FTL messages. This works well for simple copies. When the fluent strings are a ...., the full expressiveness of Fluent migrations comes to help.

Or so.

::: intl/l10n/docs/fluent_migrations.rst:178
(Diff revision 1)
> +syntax into an array of FTL messages, unfortunately it’s suitable only for
> +cases where legacy strings can be copied without any changes.
> +
> +The example above is equivalent to the following syntax, which requires a deeper
> +understanding of the underlying AST structure, and it’s also the only syntax
> +available for more complex recipes:

I'd remove the pessimistic latter half of that sentence here.

::: intl/l10n/docs/fluent_migrations.rst:208
(Diff revision 1)
> +:js:`findbar-next`. A message can have an array of attributes, each with an ID
> +and a value: in this case there is only one attribute, with ID :js:`tooltiptext`
> +and :js:`value` copied from the legacy string.
> +
> +Notice how both the ID of the message and the ID of the attribute are
> +defined as an :python:`FTL.Identifier`, not simply as a string.

Watch out for https://github.com/projectfluent/fluent/issues/144.

::: intl/l10n/docs/fluent_migrations.rst:215
(Diff revision 1)
> +
> +.. tip::
> +
> +  It’s possible to concatenate arrays of Transforms manually defined, like in
> +  the last example, with those coming from :python:`transforms_from`, by using
> +  the :python:`+` operator.

Should we recommend this?

I wonder if just doing multiple ctx.add_transforms is easier to digest?

::: intl/l10n/docs/fluent_migrations.rst:387
(Diff revision 1)
> +.. code-block:: properties
> +
> +  # LOCALIZATION NOTE (disableContainersOkButton): Semi-colon list of plural forms.
> +  # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +  # #S is the number of container tabs
> +  disableContainersOkButton = Close #S Container Tab;Close #S Container Tabs

#1

::: intl/l10n/docs/fluent_migrations.rst:416
(Diff revision 1)
> +          "disableContainersOkButton",
> +          EXTERNAL_ARGUMENT("tabCount"),
> +          lambda text: REPLACE_IN_TEXT(
> +              text,
> +              {
> +                  "#S": EXTERNAL_ARGUMENT("tabCount")

#1

::: intl/l10n/docs/fluent_migrations.rst:424
(Diff revision 1)
> +      )
> +  )
> +
> +
> +The `PLURALS` Transform will take care of creating the correct number of plural
> +categories for each language. Notice how `#S` is replaced for each of these

#1

::: intl/l10n/docs/fluent_migrations.rst:514
(Diff revision 1)
> +
> +
> +How to Test Migration Recipes
> +=============================
> +
> +Unfortunately, testing migration recipes requires several manual steps. We plan

Link to bug 1353680 here?

::: intl/l10n/docs/fluent_migrations.rst:524
(Diff revision 1)
> +1. Install Fluent Migration
> +---------------------------
> +
> +The first step is to install the `Fluent Migration`_ Python library. It’s
> +currently not available as a package, so the repository must be cloned locally
> +and installed manually, e.g. with :bash:`pip install -e .`.

Maybe we should just fix that ;-)

::: intl/l10n/docs/fluent_migrations.rst:548
(Diff revision 1)
> +
> +  hg clone https://hg.mozilla.org/l10n/gecko-strings en-US
> +  cp -r en-US test
> +
> +
> +3. Add New FTL Strings to The Local en-US Repository

I'd not try to do title case here. I think that New, The, Local might not want that.
Attachment #8984195 - Flags: review?(l10n) → review-
Comment on attachment 8984195 [details]
Bug 1467183 - Add documentation for Fluent migrations

https://reviewboard.mozilla.org/r/249996/#review256306

I didn't have time to finish reviwing all of it, but it looks amazing and I'm sure it will be very useful going forward. Great work, flod! I finished reviwng around line 200, and I need to go now, so I'm going to submit this unfinished review and come back again tomorrow. Please treat my comments as suggestions and feel free to resolve them if you disagree.

::: intl/l10n/docs/fluent_migrations.rst:15
(Diff revision 1)
> +==================================
> +Migrating Legacy Strings to Fluent
> +==================================
> +
> +Firefox is a project localized in over 100 languages. As the code for existing
> +features moves away from the old localization systems and start using `Fluent`_,

Grammar: starts

::: intl/l10n/docs/fluent_migrations.rst:16
(Diff revision 1)
> +Migrating Legacy Strings to Fluent
> +==================================
> +
> +Firefox is a project localized in over 100 languages. As the code for existing
> +features moves away from the old localization systems and start using `Fluent`_,
> +we need to ensure that we don’t lose existing translations, forcing contributors

..that we don't lose existing translations which would have the adverse effect of...

Otherwise "forcing" might be understood as a qualifier to "ensure".

::: intl/l10n/docs/fluent_migrations.rst:22
(Diff revision 1)
> +to localize hundreds of strings from scratch.
> +
> +`Fluent Migration`_ is a Python library designed to solve this specific problem:
> +it allows to migrate legacy translations from `.dtd` and `.properties` files,
> +not only moving strings and transforming them as needed to adapt to the `FTL`
> +syntax, but also replicating in VCS the history of each string.

...syntax. It also preserves some attribution history for each string in the VCS.

::: intl/l10n/docs/fluent_migrations.rst:35
(Diff revision 1)
> +involved, transformations to apply, etc. These recipes are stored in
> +`mozilla-central`__.
> +
> +__ https://hg.mozilla.org/mozilla-central/file/default/python/l10n/fluent_migrations
> +
> +When a feature is migrated to Fluent, a migration recipe should be attached to

When part of Firefox's UI is migrated...

::: intl/l10n/docs/fluent_migrations.rst:45
(Diff revision 1)
> +
> + - New Fluent strings land in `mozilla-central`, together with a migration
> +   recipe.
> + - `gecko-strings-quarantine`_ – a unified repository including strings
> +   for all shipping versions of Firefox, used as a buffer before exposing
> +   strings to localizers – is updated to include the new strings.

I'd try to rephrase this sentence in active voice:

    The new strings lands in gsq, a unified...

::: intl/l10n/docs/fluent_migrations.rst:49
(Diff revision 1)
> +   for all shipping versions of Firefox, used as a buffer before exposing
> +   strings to localizers – is updated to include the new strings.
> + - Migration recipes are run against all l10n repositories, migrating strings
> +   from old to new files, and storing them in VCS.
> + - New en-US strings are pushed to the official `gecko-strings`_ repository
> +   used by localization tools, and exposed to all localizers.

Perhaps add an explicit step here which says that after this step, the migrations end their life cycle? I know this is explained in the paragraph below, but I think it would make sense to make it in to the last bullet point on this list.

::: intl/l10n/docs/fluent_migrations.rst:154
(Diff revision 1)
> +
> +In this case there is only one Transform that migrates the string with ID
> +:js:`next.tooltip` from :bash:`toolkit/chrome/global/findbar.dtd`, and
> +inject it in the FTL fragment. The :python:`COPY` Transform allows to copy the
> +string from an existing file as is, while :python:`from_path` is used to avoid
> +repeating the same path multiple times, making the recipe less readable. Without

Here again, I'd rather say "making the recipe more readable" because I associate this phrase with "is used to avoid" :) Am I reading this wrong?

::: intl/l10n/docs/fluent_migrations.rst:173
(Diff revision 1)
> +
> +
> +This method of writing migration recipes allows to take the original FTL
> +strings, and simply replace the value of each message with a :python:`COPY`
> +Transform. :python:`transforms_from` takes care of converting the FTL-like
> +syntax into an array of FTL messages, unfortunately it’s suitable only for

...of converting the FTL syntax into an array of Transforms describing how the legacy translations should be migrated. This manner of defining migrations is only suitable to simple strings where a copy operation is sufficient. For more complex use-cases which require some additional logic in Python, you'll need to resort to the raw AST.

::: intl/l10n/docs/fluent_migrations.rst:192
(Diff revision 1)
> +          FTL.Message(
> +              id=FTL.Identifier("findbar-next"),
> +              attributes=[
> +                  FTL.Attribute(
> +                      FTL.Identifier("tooltiptext"),
> +                      COPY(

Perhaps add the names of the keyword args (id, value) to make this snippet of the AST a little bit easier to understand?

::: intl/l10n/docs/fluent_migrations.rst:208
(Diff revision 1)
> +:js:`findbar-next`. A message can have an array of attributes, each with an ID
> +and a value: in this case there is only one attribute, with ID :js:`tooltiptext`
> +and :js:`value` copied from the legacy string.
> +
> +Notice how both the ID of the message and the ID of the attribute are
> +defined as an :python:`FTL.Identifier`, not simply as a string.

I'm not sure if this sentence brings much value. Perhaps something more general would work better? Unfortunately, the best reference right now is https://github.com/projectfluent/fluent/tree/master/syntax. I'll need to update the Playground to use the refparser once Syntax 0.6 is published.

Also note that we might be removing the Identifier nodes in the near future, so if we keep this sentence, we'll need to make sure we update it later on. See https://github.com/projectfluent/fluent/issues/144.

::: intl/l10n/docs/fluent_migrations.rst:213
(Diff revision 1)
> +defined as an :python:`FTL.Identifier`, not simply as a string.
> +
> +
> +.. tip::
> +
> +  It’s possible to concatenate arrays of Transforms manually defined, like in

...defined manually...
(In reply to Axel Hecht [:Pike] from comment #5)
> ::: intl/l10n/docs/fluent_migrations.rst:120
> (Diff revision 1)
> > +  def migrate(ctx):
> > +      """Bug 1411707 - Migrate the findbar XBL binding to a Custom Element, part {index}."""
> > +
> > +      ctx.add_transforms(
> > +          "toolkit/toolkit/main-window/findbar.ftl",
> > +          "toolkit/toolkit/main-window/findbar.ftl",
> 
> I wouldn't mind if this was just `toolkit/main-window/findbar.ftl`.
> Documenting `bla/bla` seems to set a bad incentive.

This is an actual migration (bug 1411707), and it matches the `/browser/browser`.


> ::: intl/l10n/docs/fluent_migrations.rst:215
> (Diff revision 1)
> > +
> > +.. tip::
> > +
> > +  It’s possible to concatenate arrays of Transforms manually defined, like in
> > +  the last example, with those coming from :python:`transforms_from`, by using
> > +  the :python:`+` operator.
> 
> Should we recommend this?
> 
> I wonder if just doing multiple ctx.add_transforms is easier to digest?

Added that there's an alternative, but personally I find it readable
https://dxr.mozilla.org/mozilla-central/rev/f7fd9b08c0156be5b5cd99de5ed0ed0b98d93051/python/l10n/fluent_migrations/bug_1457021_js_subdialogs.py

> 
> ::: intl/l10n/docs/fluent_migrations.rst:387
> (Diff revision 1)
> > +.. code-block:: properties
> > +
> > +  # LOCALIZATION NOTE (disableContainersOkButton): Semi-colon list of plural forms.
> > +  # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> > +  # #S is the number of container tabs
> > +  disableContainersOkButton = Close #S Container Tab;Close #S Container Tabs
> 
> #1

This is an actual migration, but I guess it makes no point using a broken string as an example.

> ::: intl/l10n/docs/fluent_migrations.rst:524
> (Diff revision 1)
> > +1. Install Fluent Migration
> > +---------------------------
> > +
> > +The first step is to install the `Fluent Migration`_ Python library. It’s
> > +currently not available as a package, so the repository must be cloned locally
> > +and installed manually, e.g. with :bash:`pip install -e .`.

Yes. Happy to update the doc when that happens :)

Addressing all other comments (hopefully).
(In reply to Staś Małolepszy :stas from comment #6)
> ::: intl/l10n/docs/fluent_migrations.rst:22
> (Diff revision 1)
> > +to localize hundreds of strings from scratch.
> > +
> > +`Fluent Migration`_ is a Python library designed to solve this specific problem:
> > +it allows to migrate legacy translations from `.dtd` and `.properties` files,
> > +not only moving strings and transforming them as needed to adapt to the `FTL`
> > +syntax, but also replicating in VCS the history of each string.
> 
> ...syntax. It also preserves some attribution history for each string in the
> VCS.

Used Pike's suggestion here.

> ::: intl/l10n/docs/fluent_migrations.rst:45
> (Diff revision 1)
> > +
> > + - New Fluent strings land in `mozilla-central`, together with a migration
> > +   recipe.
> > + - `gecko-strings-quarantine`_ – a unified repository including strings
> > +   for all shipping versions of Firefox, used as a buffer before exposing
> > +   strings to localizers – is updated to include the new strings.
> 
> I'd try to rephrase this sentence in active voice:
> 
>     The new strings lands in gsq, a unified...

I'm having a hard time switching this to active mode, mostly because all steps but the first require manual actions from l10n-drivers.

> ::: intl/l10n/docs/fluent_migrations.rst:49
> (Diff revision 1)
> > +   for all shipping versions of Firefox, used as a buffer before exposing
> > +   strings to localizers – is updated to include the new strings.
> > + - Migration recipes are run against all l10n repositories, migrating strings
> > +   from old to new files, and storing them in VCS.
> > + - New en-US strings are pushed to the official `gecko-strings`_ repository
> > +   used by localization tools, and exposed to all localizers.
> 
> Perhaps add an explicit step here which says that after this step, the
> migrations end their life cycle? I know this is explained in the paragraph
> below, but I think it would make sense to make it in to the last bullet
> point on this list.

This is partially true though, since we run these recipes for a few more time in the cycle. Adding some text though (also moved this section at the bottom of the doc)

> 
> ::: intl/l10n/docs/fluent_migrations.rst:154
> (Diff revision 1)
> > +
> > +In this case there is only one Transform that migrates the string with ID
> > +:js:`next.tooltip` from :bash:`toolkit/chrome/global/findbar.dtd`, and
> > +inject it in the FTL fragment. The :python:`COPY` Transform allows to copy the
> > +string from an existing file as is, while :python:`from_path` is used to avoid
> > +repeating the same path multiple times, making the recipe less readable. Without
> 
> Here again, I'd rather say "making the recipe more readable" because I
> associate this phrase with "is used to avoid" :) Am I reading this wrong?

Nope, you're correct.

> 
> ::: intl/l10n/docs/fluent_migrations.rst:208
> (Diff revision 1)
> > +:js:`findbar-next`. A message can have an array of attributes, each with an ID
> > +and a value: in this case there is only one attribute, with ID :js:`tooltiptext`
> > +and :js:`value` copied from the legacy string.
> > +
> > +Notice how both the ID of the message and the ID of the attribute are
> > +defined as an :python:`FTL.Identifier`, not simply as a string.
> 
> I'm not sure if this sentence brings much value. Perhaps something more
> general would work better? Unfortunately, the best reference right now is
> https://github.com/projectfluent/fluent/tree/master/syntax. I'll need to
> update the Playground to use the refparser once Syntax 0.6 is published.
> 
> Also note that we might be removing the Identifier nodes in the near future,
> so if we keep this sentence, we'll need to make sure we update it later on.
> See https://github.com/projectfluent/fluent/issues/144.

Personally I don't think it's intuitive that this can't be a simple string, that's why I'm calling it out.

Addressed the comments, not sure if I didn't break the syntax (don't have mach on this machine, will need to check tomorrow).
Comment on attachment 8984195 [details]
Bug 1467183 - Add documentation for Fluent migrations

https://reviewboard.mozilla.org/r/249996/#review257030

This looks good to me, I resolved the issues that I think are resolved. I couldn't do that for stas' comments, and there might be one open still about using `id` and `value` for one of the Attribute nodes?

Commonly the patch author goes in and resolves the issues in the updated patch, and then the reviewer just verifies that, btw.
Attachment #8984195 - Flags: review?(l10n) → review+
(In reply to Axel Hecht [:Pike] from comment #12)
> Commonly the patch author goes in and resolves the issues in the updated
> patch, and then the reviewer just verifies that, btw.

Sorry about that, I normally close them before landing (and I've seen it done that way in other cases), but I see how it makes more sense in case of re-review.
Comment on attachment 8984195 [details]
Bug 1467183 - Add documentation for Fluent migrations

https://reviewboard.mozilla.org/r/249996/#review257036

::: intl/l10n/docs/fluent_migrations.rst:273
(Diff revision 4)
> +
> +  value=REPLACE(
> +      "browser/chrome/browser/preferences/preferences.properties",
> +      "searchResults.sorryMessageWin",
> +      {
> +          "%S": FTL.TextElement("<span data-l10n-name="query"></span>")

This example will not work because of the use of `"` mark both to encode a string and within it. You may have to switch to `'` or escape them.

::: intl/l10n/docs/fluent_migrations.rst:296
(Diff revision 4)
> +
> +Concatenating Strings
> +---------------------
> +
> +It’s quite common to concatenate multiple strings coming from `DTD` and
> +`properties`, for example to create sentences with HTML markup. It’s possible to

I think there's an opportunity here to reference Fluent best practies on why you should concatenate here and not at runtime - https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_tutorial.html#social-contract
Attachment #8984195 - Flags: review?(gandalf) → review+
Looks great Flod! Thanks so much for writing this down :)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #14)
> Comment on attachment 8984195 [details]
> Bug 1467183 - Add documentation for Fluent migrations,
> 
> https://reviewboard.mozilla.org/r/249996/#review257036
> 
> ::: intl/l10n/docs/fluent_migrations.rst:273
> (Diff revision 4)
> > +
> > +  value=REPLACE(
> > +      "browser/chrome/browser/preferences/preferences.properties",
> > +      "searchResults.sorryMessageWin",
> > +      {
> > +          "%S": FTL.TextElement("<span data-l10n-name="query"></span>")
> 
> This example will not work because of the use of `"` mark both to encode a
> string and within it. You may have to switch to `'` or escape them.

Uh, you're absolutely right. I replaced quotes for consistency, and didn't think about this. Fixing it.


> ::: intl/l10n/docs/fluent_migrations.rst:296
> (Diff revision 4)
> > +
> > +Concatenating Strings
> > +---------------------
> > +
> > +It’s quite common to concatenate multiple strings coming from `DTD` and
> > +`properties`, for example to create sentences with HTML markup. It’s possible to
> 
> I think there's an opportunity here to reference Fluent best practies on why
> you should concatenate here and not at runtime -
> https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_tutorial.
> html#social-contract

Good point. I need to figure out how to link to a section of another doc, for now linking to the doc itself. Also need to figure out that the syntax works, as soon as I have the build system available.
Comment on attachment 8984195 [details]
Bug 1467183 - Add documentation for Fluent migrations

https://reviewboard.mozilla.org/r/249996/#review257052

::: intl/l10n/docs/fluent_migrations.rst:300
(Diff revisions 4 - 5)
>  It’s quite common to concatenate multiple strings coming from `DTD` and
>  `properties`, for example to create sentences with HTML markup. It’s possible to
> -concatenate strings and text element in a migration recipe using the
> -:python:`CONCAT` Transform.
> +concatenate strings and text elements in a migration recipe using the
> +:python:`CONCAT` Transform. This allows to generate a single Fluent message from
> +these fragments, avoiding run-time transformations as prescribed by
> +:doc:`Fluent’s social contract <fluent_tutorial>`.

This should link to the anchor, and make sure it works when building docs locally.
Comment on attachment 8984195 [details]
Bug 1467183 - Add documentation for Fluent migrations

https://reviewboard.mozilla.org/r/249996/#review258190

Amazing work, flod, putting all this know-how into text!

In places where I suggest to add a sentence, my suggestions are only that, suggestions. Feel free to rephrase as you see fit.

::: intl/l10n/docs/fluent_migrations.rst:91
(Diff revision 6)
> +
> +  findbar-next =
> +      .tooltiptext = Find the next occurrence of the phrase
> +
> +
> +This is how the migration recipe looks like:

Grammar nit: I've been tought that it's either "How does it look" or "What does it look like". There also a subtle difference in meaning: the first one is asking for an adverb: it looks good/bad while the second one is asking for a description.

::: intl/l10n/docs/fluent_migrations.rst:142
(Diff revision 6)
> + - An array of Transforms. Transforms are AST nodes which describe how legacy
> +   translations should be migrated.
> +
> +In this case there is only one Transform that migrates the string with ID
> +:js:`next.tooltip` from :bash:`toolkit/chrome/global/findbar.dtd`, and
> +inject it in the FTL fragment. The :python:`COPY` Transform allows to copy the

Nit: injects

::: intl/l10n/docs/fluent_migrations.rst:208
(Diff revision 6)
> +.. tip::
> +
> +  It’s possible to concatenate arrays of Transforms defined manually, like in
> +  the last example, with those coming from :python:`transforms_from`, by using
> +  the :python:`+` operator. Alternatively, it’s possible to use multiple
> +  :python:`add_transforms`.

It might be a good idea to mention here that the order of transforms doesn't matter. The reference file is used for ordering.

::: intl/l10n/docs/fluent_migrations.rst:278
(Diff revision 6)
> +          "%S": FTL.TextElement('<span data-l10n-name="query"></span>')
> +      }
> +  )
> +
> +
> +.. note::

I'd add here at the beginning of the note:

    EXTERNAL_ARGUMENT and MESSAGE_REFERENCE are helper transforms which can be used to save keystrokes in common cases using the raw AST is too verbose.

::: intl/l10n/docs/fluent_migrations.rst:300
(Diff revision 6)
> +It’s quite common to concatenate multiple strings coming from `DTD` and
> +`properties`, for example to create sentences with HTML markup. It’s possible to
> +concatenate strings and text elements in a migration recipe using the
> +:python:`CONCAT` Transform. This allows to generate a single Fluent message from
> +these fragments, avoiding run-time transformations as prescribed by
> +:ref:`Fluent’s social contract <fluent-tutorial-social-contract>`.

I'd add here:

    Note that in the case of simple migrations using `transforms_from`, the concatenation is carried out implicitly by the fact of using the Fluent syntax interleaved with COPY() transform calls to define the migration recipe.

::: intl/l10n/docs/fluent_migrations.rst:335
(Diff revision 6)
> +      value=REPLACE(
> +          "browser/chrome/browser/preferences/preferences.properties",
> +          "searchResults.needHelp",
> +          {
> +              "%S": CONCAT(
> +                  FTL.TextElement("<a data-l10n-name="url">"),

Double quotes around "url" need to be escaped here.

::: intl/l10n/docs/fluent_migrations.rst:421
(Diff revision 6)
> +
> +
> +The `PLURALS` Transform will take care of creating the correct number of plural
> +categories for each language. Notice how `#1` is replaced for each of these
> +variants with :js:`{ $tabCount }`, using :python:`REPLACE_IN_TEXT` and
> +:python:`EXTERNAL_ARGUMENT("tabCount")`.

I'd add here:

    We can't use REPLACE here because it takes a file path and sa string id as arguments, whereas here we operate on regular text. We perform the replacement on each plural form of the original string separated by the semicolon.

::: intl/l10n/docs/fluent_migrations.rst:457
(Diff revision 6)
> +  FTL.Message(
> +      id=FTL.Identifier("use-current-pages"),
> +      attributes=[
> +          FTL.Attribute(
> +              FTL.Identifier("label"),
> +              FTL.Pattern(

Please use "id" and "value" as argument names here.

::: intl/l10n/docs/fluent_migrations.rst:464
(Diff revision 6)
> +                      FTL.Placeable(
> +                          expression=FTL.SelectExpression(
> +                              expression=EXTERNAL_ARGUMENT("tabCount"),
> +                              variants=[
> +                                  FTL.Variant(
> +                                      key=FTL.VariantName("1"),

I think this should be an FTL.Number(1)?

::: intl/l10n/docs/fluent_migrations.rst:487
(Diff revision 6)
> +                  ]
> +              )
> +          ),
> +          FTL.Attribute(
> +              FTL.Identifier("accesskey"),
> +              COPY(

Explicit "id" and "value" here as well, please.

::: intl/l10n/docs/fluent_migrations.rst:499
(Diff revision 6)
> +
> +
> +This Transform uses several concepts already described in this document. Notable
> +new elements are:
> +
> + - The fact that the `label` attribute is defined as a :python:`Pattern`.

I'd add after this sentence:

    It's because in this example we're creating a new value from scratch and migrating existing translations as its variants. Patterns are one of Fluent's value types.

::: intl/l10n/docs/fluent_migrations.rst:531
(Diff revision 6)
> +2. Clone gecko-strings
> +----------------------
> +
> +Migration recipes work against localization repositories, which means it’s not
> +possible to test them directly against `mozilla-central`, unless the *source*
> +path in :python:`ctx.add_transforms` is temporarily tweaked to match

...unless the source path (the second argument)...

::: intl/l10n/docs/fluent_migrations.rst:535
(Diff revision 6)
> +possible to test them directly against `mozilla-central`, unless the *source*
> +path in :python:`ctx.add_transforms` is temporarily tweaked to match
> +`mozilla-central` paths.
> +
> +To test the actual recipe that will land in the patch, it’s necessary to clone
> +the `gecko-strings`_ repository on the system twice, in two separate folders.

...One will simulate the reference en-US repository after the patch has landed, and the other will simulate a target localization...

::: intl/l10n/docs/fluent_migrations.rst:574
(Diff revision 6)
> +    name_of_the_recipe
> +
> +
> +:bash:`PYTHONPATH` is updated to include the folder storing migration recipes.
> +The name of the recipe needs to be specified without the :bash:`.py` extension,
> +since it’s imported as a module.

Maybe just recommend to `cd PATH/TO/recipes` rather than setting the PYTHONPATH?

::: intl/l10n/docs/fluent_migrations.rst:676
(Diff revision 6)
> +
> +Don’t hesitate to reach out to the l10n-drivers for feedback, help to test or
> +write the migration recipes:
> +
> + - Francesco Lodolo (:flod)
> + - Stas Malolepszy (:stas)

Any chance I could get my funny Polish letters here? :)

    Staś Małolepszy
Attachment #8984195 - Flags: review?(stas) → review+
Comment on attachment 8984195 [details]
Bug 1467183 - Add documentation for Fluent migrations

https://reviewboard.mozilla.org/r/249996/#review258190

> I think this should be an FTL.Number(1)?

This is how the original recipe was written
https://github.com/flodolo/fluent-migrations/blob/master/recipes/fx60/bug_1435912_preferences_general_xul.py#L219

I assume they both produce the same result, but isn't more logic to use `FTL.VariantName`, since we're build a variant?
(In reply to Francesco Lodolo [:flod] from comment #21)

> I assume they both produce the same result, but isn't more logic to use
> `FTL.VariantName`, since we're build a variant?

Variant keys are of type VariantKey, which can be either a VariantName (when it's text) or a NumberExpression (renamed to NumberLiteral in Syntax 0.6; not a "Number" like I mistakenly wrote above). The distinction is an important one in the runtime: it's how the resolver can know to treat a key as a number and apply special logic to it. As you say, for serialization this doesn't really matter, but semantically we should use NumberExpression(1) here.
Pushed by francesco.lodolo@mozillaitalia.org:
https://hg.mozilla.org/integration/autoland/rev/97a8832286af
Add documentation for Fluent migrations, r=gandalf,Pike,stas
https://hg.mozilla.org/mozilla-central/rev/97a8832286af
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.