Handle plural gen_l10n regular placeholders and DateTime placeholders by shihaohong · Pull Request #47483 · flutter/flutter

shihaohong · 2019-12-19T22:06:12Z

Description

Adds plural placeholder handling in the gen_l10n tool. It also accounts for plural DateTime processing.

Related Issues

Fixes #47368
Related to #47008

Tests

I added the following tests:

A test to ensure that the count placeholder and a regular placeholder are properly differentiated.
A test to ensure that DateTime processing happens for plural messages
A few tests to ensure that that placeholders resource attribute for plural messages are properly formatted and that appropriate errors are thrown when they are not

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.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: Replace with a link to your migration guide

HansMuller · 2019-12-19T23:31:35Z

Date valued parameters?

shihaohong · 2019-12-20T00:04:40Z

@HansMuller Let me go ahead date params for plurals a part of this PR as well

HansMuller · 2019-12-20T15:47:50Z

+      return 'Object $parameter';
+    }).toList();
+  }
+  return <String>[];


If we get here, then we're not going to generate a correct plural method? Should exit with an error.

HansMuller · 2019-12-20T15:52:42Z

+
+  // Used to determine which placeholder is the plural count placeholder
+  final String resourceValue = bundle[key] as String;
+  final String countPlaceholder = resourceValue.split(',')[0].substring(1);


This will fail in a confusing way if the plurals string isn't formatted correctly. Maybe check the resourceValue string first, and exit with an error if it doesn't appear to be formatted correctly, i.e. if it looks like resourceValue.split(',')[0].substring(1) would fail.

I tried creating an exception here and writing a test to error out if:

If the first character was not '{'

If resourceValue.split(',').length <= 2

However, it seems that the string is not parsed as a plural message earlier in the tool (it is instead treated as a simple message). So, it never reaches these checks I added. Can you think of any other situation where the plurals string might be formatted incorrectly here?

You're right - we only get here if the resource matches pluralValueRE, which requires the string to contain fooVar,plural. There are probably other ways for plural strings to go wrong, but I don't think that needs to be addressed in this PR.

shihaohong · 2019-12-20T17:48:46Z

  static RegExp arbFilenameLocaleRE = RegExp(r'^[^_]*_(\w+)\.arb$');
  static RegExp arbFilenameRE = RegExp(r'(\w+)\.arb$');
-  static RegExp pluralValueRE = RegExp(r'^\s*\{[\w\s,]*,\s*plural\s*,');
+  static RegExp pluralValueRE = RegExp(r'^\s*\{[\w\s]*,\s*plural\s*,[\s\S]*\}');


I thought of maybe making this a little more precise to help avoid the plural string formatting issue mentioned before. However, I was wondering if this would somehow make the match too strict? @HansMuller

I think this is OK as it is. We'll have to see what failure modes actually crop up in real apps before adding more checking.

Sounds good, I'll change it to be what it was before

HansMuller

LGTM

HansMuller · 2019-12-20T18:14:48Z

+
+  // Used to determine which placeholder is the plural count placeholder
+  final String resourceValue = bundle[key] as String;
+  final String countPlaceholder = resourceValue.split(',')[0].substring(1);


You're right - we only get here if the resource matches pluralValueRE, which requires the string to contain fooVar,plural. There are probably other ways for plural strings to go wrong, but I don't think that needs to be addressed in this PR.

HansMuller · 2019-12-20T18:16:00Z

  static RegExp arbFilenameLocaleRE = RegExp(r'^[^_]*_(\w+)\.arb$');
  static RegExp arbFilenameRE = RegExp(r'(\w+)\.arb$');
-  static RegExp pluralValueRE = RegExp(r'^\s*\{[\w\s,]*,\s*plural\s*,');
+  static RegExp pluralValueRE = RegExp(r'^\s*\{[\w\s]*,\s*plural\s*,[\s\S]*\}');


I think this is OK as it is. We'll have to see what failure modes actually crop up in real apps before adding more checking.

shihaohong added 2 commits December 19, 2019 13:48

Wrote plural placeholder test

f3da023

Handle placeholders in plurals

40111be

shihaohong added the a: internationalization Supporting other languages or locales. (aka i18n) label Dec 19, 2019

shihaohong requested a review from HansMuller December 19, 2019 22:06

fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Dec 19, 2019

googlebot added the cla: yes label Dec 19, 2019

shihaohong added the work in progress; do not review label Dec 20, 2019

shihaohong added 2 commits December 19, 2019 17:46

Add DateTime plural placeholder test

0c61d87

Date plural placeholders

363f224

shihaohong removed the work in progress; do not review label Dec 20, 2019

shihaohong mentioned this pull request Dec 20, 2019

Handling DateTime, Currency and Numbers in gen_l10n tool #47008

Closed

HansMuller reviewed Dec 20, 2019

View reviewed changes

shihaohong added 4 commits December 20, 2019 09:00

Add guards for improper plural placeholder arb format

02cb06f

Improve variable names, improve RegExp plural matching

456de29

Add back whitespace regexp matcher to plural regexp

35fd82c

Code cleanup

bdfff4c

shihaohong commented Dec 20, 2019

View reviewed changes

Turn assert into exceptions, add tests

f91a296

shihaohong changed the title ~~Plural placeholders~~ Handle plural gen_l10n regular placeholders and DateTime placeholders Dec 20, 2019

Fix name of test

4825a45

HansMuller approved these changes Dec 20, 2019

View reviewed changes

shihaohong added 2 commits December 20, 2019 10:49

revert plural regexp

a636977

Fix typo in error message

7c48a07

shihaohong merged commit 88cd113 into flutter:master Dec 21, 2019

shihaohong deleted the plural-placeholders branch December 21, 2019 00:22

github-actions Bot locked as resolved and limited conversation to collaborators Aug 2, 2021

Conversation

shihaohong commented Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

HansMuller commented Dec 19, 2019

Uh oh!

shihaohong commented Dec 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shihaohong commented Dec 19, 2019 •

edited

Loading