Skip to content

[gen_l10n] Warn users when placeholder types are converted to 'num' when using pluralization #108036

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
Merged
merged 7 commits into from
Jul 26, 2022
Merged

[gen_l10n] Warn users when placeholder types are converted to 'num' when using pluralization #108036

merged 7 commits into from
Jul 26, 2022

Conversation

thkim1011
Copy link
Contributor

@thkim1011 thkim1011 commented Jul 20, 2022 •
edited
Loading

See title. This PR refactors logging so that the logger is saved as an instance variable of the LocalizationsGenerator class, and adds a warning when there is a pluralization placeholder whose type isn't num.

Fixes #98868

Tested locally:

Multiple devices found:
macOS (desktop) • macos  • darwin-arm64   • macOS 12.4 21F79 darwin-arm (Rosetta)
Chrome (web)    • chrome • web-javascript • Google Chrome 103.0.5060.114
[1]: macOS (macos)
[2]: Chrome (chrome)
Please choose one (To quit, press "q/Q"): 2
Placeholders for plurals are automatically converted to type 'num'.
Placeholders for plurals are automatically converted to type 'num'.
Running "flutter pub get" in i18n_test...                          698ms

I didn't think I needed to add tests here since there's really no change other than logging.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 20, 2022
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@thkim1011 thkim1011 requested a review from HansMuller July 20, 2022 17:41
HansMuller
HansMuller reviewed Jul 20, 2022
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.

You can ask @Hixie for a "test exemption" however it might be simplest to just include one.

packages/flutter_tools/lib/src/localizations/gen_l10n.dart Outdated
if (message.isPlural) {
final Placeholder countPlaceholder = message.getCountPlaceholder();
if (countPlaceholder.type != null && countPlaceholder.type != 'num') {
logger.printWarning("Placeholders for plurals are automatically converted to type 'num'.");
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be difficult for the developer to correlate this warning with a specific message. It would be helpful to include the name of the message and the name of the parameter in the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@thkim1011
Copy link
Contributor Author

Hey @HansMuller, I just made the change you recommended, added a test (turns out testing logs are not too bad), and fixed up the rest of the tests that were affected by changing the LocalizationsGenerator constructor.

@thkim1011 thkim1011 requested a review from HansMuller July 25, 2022 23:28
HansMuller
HansMuller approved these changes Jul 26, 2022
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller 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.

The bots noticed some trailing spaces. Other than that and a trivial point about a comment, this is ready to land.

packages/flutter_tools/lib/src/localizations/gen_l10n.dart Outdated
@@ -1141,6 +1148,22 @@ class LocalizationsGenerator {
);
});

// Warn users when placeholder types are overrided to 'num'.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment probably isn't necessary; at the moment it doesn't describe the entire block.

packages/flutter_tools/lib/src/localizations/gen_l10n.dart
@@ -1141,6 +1148,22 @@ class LocalizationsGenerator {
);
});

// Warn users when placeholder types are overrided to 'num'.
for (final Message message in messages) {
if (message.isPlural) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests/warnings are great.

@thkim1011 thkim1011 added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 26, 2022

  • The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2022
@thkim1011 thkim1011 added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 26, 2022

  • The status or check suite Windows build_tests_1_3 has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2022
@thkim1011 thkim1011 merged commit 001c499 into flutter:master Jul 26, 2022
@thkim1011 thkim1011 deleted the tae/log-warning branch July 26, 2022 21:13
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 27, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 27, 2022
@engine-flutter-autoroll
001c499 [gen_l10n] Warn users when placeholder types are converted to…
6d9a7b3 
… 'num' when using pluralization (flutter/flutter#108036)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jul 28, 2022
@engine-flutter-autoroll
001c499 [gen_l10n] Warn users when placeholder types are converted to…
710da4f 
… 'num' when using pluralization (flutter/flutter#108036)
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
@thkim1011 @camsim99
[gen_l10n] Warn users when placeholder types are converted to 'num' w…
0a67da9 
…hen using pluralization (flutter#108036)

* init

* asdf

* fix tests

* change warning slightly

* fix test again

* fix spacing and comment

* fix lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HansMuller HansMuller HansMuller approved these changes

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.

[[gen_l10n]]: Pluralization in arb files is not producing expected output
2 participants
@thkim1011 @HansMuller