Skip to content

gen_l10n: support string list as preferred-supported-locales, as documented#63649

Merged
shihaohong merged 3 commits intoflutter:masterfrom
hpoul:gen_l10n_preferred_locale_list
Sep 9, 2020
Merged

gen_l10n: support string list as preferred-supported-locales, as documented#63649
shihaohong merged 3 commits intoflutter:masterfrom
hpoul:gen_l10n_preferred_locale_list

Conversation

@hpoul
Copy link
Contributor

@hpoul hpoul commented Aug 13, 2020

Description

according to https://docs.google.com/document/d/10e0saTfAv32OZLRmONy866vnaw0I2jwL8zukykpgWBc/edit

The list of preferred supported locales for the application. By default, the tool will generate the supported locales list in alphabetical order. Use this flag if you would like to default to a different locale.

For example, pass in ['en_US'] if you would like your app to default to American English if a device supports it.

but actually the yaml expects a string at that position, and the gen_l10n tool expects a valid json. so the yaml file would have to contain something like:

preferred-supported-locales: '["en_US"]'

Also, the help in gen_l10n documents the --preferred-supported-locales also as ['en_US'] but uses json.decode which only works with ["en_US"].

fwiw: imho it is a bit weird that --preferred-supported-locales is passed as json. I thought about changing gen_l10n to make the --preferred-supported-locales a allowMultiple: true option. This seems to be more natural for a command line app, but this would probably break anyone using it this way?

Tests

I added the following tests:

  • Test to make sure list in yaml file is parsed correctly.

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [Handling breaking changes].

  • No, no existing tests failed, so this is not a breaking change.

@flutter-dashboard flutter-dashboard bot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Aug 13, 2020
@hpoul hpoul force-pushed the gen_l10n_preferred_locale_list branch from da62f72 to c04d017 Compare August 13, 2020 11:34
shihaohong
shihaohong reviewed Aug 13, 2020
View reviewed changes
Copy link
Contributor

@shihaohong shihaohong left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe what we really want is to represent the preferred-supported-locales option as a yaml sequence. Then, we can parse the yaml sequence into the json format desired by the gen_l10n tool.

cc/ @jonahwilliams, who might have ideas on how to best approach this.

As for the gen_l10n tool itself parsing the list of supported locales as JSON, could you further elaborate on what the allowMultiple: true flag would do? I think it'd be great to consider any ideas that would make the tool more ergonomic (if it isn't too breaking). We haven't really announced the gen_l10n tool as a stable part of the framework yet, so we might still have a window of opportunity to make it as ergonomic as possible before that happens.

@hpoul
Copy link
Contributor Author

hpoul commented Aug 13, 2020

@shihaohong this PR should be able to parse yaml sequences. the yaml parser makes no difference between flow sequences and block sequences.

I meant using the parameter multiple times for the gen_l10n cli https://pub.dev/documentation/args/latest/args/ArgParser/addMultiOption.html like:

--preferred-supported-locales=en --preferred-supported-locales=de instead of --preferred-supported-locales='["en","de"]'

one more suggestion for gen_l10n: How about defaulting preferred-supported-locales to be equal to the locale of the template-arb-file? imho in most cases, this would be the expected behavior, if none of the supported locales matches the system locale, it would take the locale from the "template" language?

@hpoul hpoul force-pushed the gen_l10n_preferred_locale_list branch from 8cc20e2 to 844ef62 Compare September 3, 2020 23:53
@hpoul
Copy link
Contributor Author

hpoul commented Sep 3, 2020

@shihaohong i did a rebase on the last master.. seems like the command line parameters have been removed 👍️ so the only remaining thing is correctly reading the collection/list from the yaml. let's see how the tests work out

@hpoul
convert preferredSupportedLocale to a list of strings. accept a multi…
3684bb3 
… option instead of a json string in the command.
@hpoul
Copy link
Contributor Author

hpoul commented Sep 4, 2020

@shihaohong I have now also changed the --preferred-supported-locales to be a multiOption and the whole api takes a List<String> instead of a String. type safety FTW ;-)

shihaohong
shihaohong approved these changes Sep 9, 2020
View reviewed changes
Copy link
Contributor

@shihaohong shihaohong left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this change looks great! Thank you for your patience, I've been working through the synthetic package work with the gen_l10n tool over the last few weeks and haven't had time to get to this PR until now.

As a quick summary, this change is breaking, but definitely improves the API for the gen_l10n tool.

For example, in l10n.yaml, this:

preferred-supported-locales: '["en", "es"]'

is now be represented like this:

preferred-supported-locales:
  - en
  - es

or

preferred-supported-locales: [en, es]

And when running from the command line, instead of:

flutter gen-l10n --preferred-supported-locales='["en", "es"]' <other options

it is:

flutter gen-l10n --preferred-supported-locales=en --preferred-supported-locales=es <other options>

^ The order of preferred supported locales matters using the multiOption arg parser, since that's the order that will be used in the tool.

cc/ @HansMuller

HansMuller
HansMuller reviewed Sep 9, 2020
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks OK to me; definitely best to be YAML-conventional. Presumably, developers will not specify more than one or two locales on the command line, since that would get tedious.

@shihaohong shihaohong merged commit 8dfd42f into flutter:master Sep 9, 2020
@shihaohong shihaohong mentioned this pull request Sep 9, 2020
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@HansMuller HansMuller HansMuller left review comments

@shihaohong shihaohong shihaohong approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hpoul @HansMuller @shihaohong @googlebot