Skip to content

[gen_l10n] support 'select' #85116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
asashour wants to merge 8 commits into from
Closed

[gen_l10n] support 'select' #85116

asashour wants to merge 8 commits into from

Conversation

@asashour
Copy link
Contributor

@asashour asashour commented Jun 23, 2021

Fixes #81981

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 23, 2021
@google-cla google-cla bot added the cla: yes label Jun 23, 2021
shihaohong
shihaohong suggested changes Jul 1, 2021
View reviewed changes
@asashour asashour force-pushed the 81981 branch 2 times, most recently from 87f01c2 to ad0b792 Compare July 7, 2021 08:18
@asashour asashour mentioned this pull request Jul 9, 2021
Merged
8 tasks
@zanderso
Copy link
Member

zanderso commented Jul 15, 2021

@HansMuller @jonahwilliams who should review this?

@asashour
Copy link
Contributor Author

asashour commented Jul 15, 2021

Please note this is now part of a bigger PR #86167.

I am leaving the team to decide, whichever lands first.

If the big one is ok, this should be closed without review, if we want to take it into steps, then this goes first.

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Jul 20, 2021

I have no context for the l10n work. I can check over the code but its up to @HansMuller for a final say

jonahwilliams
jonahwilliams reviewed Jul 20, 2021
View reviewed changes
packages/flutter_tools/test/general.shard/generate_localizations_test.dart
..childFile('app_es.arb').writeAsStringSync(selectMessageEsArb);

try {
LocalizationsGenerator(
Copy link
Contributor

@jonahwilliams jonahwilliams Jul 20, 2021

Choose a reason for hiding this comment

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

You don't need to explicitly fail here, throwing an unhandled exception will already fail the test

Copy link
Contributor Author

@asashour asashour Jul 21, 2021

Choose a reason for hiding this comment

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

Thanks for your feedback, resolved in the other PR.

jonahwilliams
jonahwilliams reviewed Jul 20, 2021
View reviewed changes
packages/flutter_tools/lib/src/localizations/gen_l10n.dart

final List<String> cases = <String>[];

final RegExp selectRE = RegExp(r'\{([\w\s,]*),\s*select\s*,\s*([\w\d]+\s*\{.*\})+\s*\}');
Copy link
Contributor

@jonahwilliams jonahwilliams Jul 20, 2021

Choose a reason for hiding this comment

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

I would hoist this regex into a static so that it is only compiled once, and additional document a bit what it is intended to match to help future contributors.

@HansMuller
Copy link
Contributor

HansMuller commented Jul 20, 2021

@jonahwilliams - this project has moved to #86167. I put a link to your comments there.

@asashour - It would be best to close this PR in favor of #86167

@asashour
Copy link
Contributor Author

asashour commented Jul 21, 2021

Closed as part of #86167

@asashour asashour closed this Jul 21, 2021
@asashour asashour deleted the 81981 branch July 21, 2021 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HansMuller HansMuller Awaiting requested review from HansMuller

2 more reviewers

@jonahwilliams jonahwilliams jonahwilliams left review comments

@shihaohong shihaohong shihaohong requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

[proposal] gen_l10n should support select

5 participants

@asashour @zanderso @jonahwilliams @HansMuller @shihaohong