-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[gen_l10n] Support plurals and selects inside string #86167
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.
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
[gen_l10n] Support plurals and selects inside string #86167
Conversation
|
It looks like this PR would also help with #81981. |
packages/flutter_tools/test/integration.shard/test_data/gen_l10n_project.dart
Show resolved
Hide resolved
| "helloWorldPopulation": "{ES - count,plural, =1{ES - Hello World of {population} citizens} =2{ES - Hello two worlds with {population} total citizens} many{ES - Hello all {count} worlds, with a total of {population} citizens} other{ES - Hello other {count} worlds, with a total of {population} citizens}}", | ||
| "helloWorldInterpolation": "ES - [{hello}] #{world}#", | ||
| "helloWorldsInterpolation": "ES - {count,plural, other {ES - [{hello}] -{world}- #{count}#}}", | ||
| "helloWorldsInterpolation": "{count,plural, other {ES - [{hello}] -{world}- #{count}#}}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has this been changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that the previous test case for helloWorldsInterpolation for ES was wrong, since we didn't process the prefix string, but now we do.
If this wasn't changed, we need to change the expectations to include this added "ES - ", but this scenario with "ES" is tested with pluralInString.
packages/flutter_tools/lib/src/localizations/gen_l10n_types.dart
Outdated
Show resolved
Hide resolved
58543f1 to
13f6eec
Compare
|
Hey there! So the other day I discovered this great PR by @asashour which helped me a lot implementing internationalization in my Flutter app! On my way, I encountered a few "gotchas". For example, defining a string containing both plural and normal placeholder generate an invalid output. I based my work on the this PR which led me to the following commit: https://github.com/Delgan/flutter/commit/1398287af59c0f49ae445606e896e17f098f819b Due to the refactor I made, I guess these modifications need to be discussed further. I just want to avoid duplicating the review and correction work if it is already taken into account in the commit I proposed. 👍 |
090cfdc to
0976fd9
Compare
HansMuller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a huge improvement!
Unless I've overlooked something (quite possible), we seem to be missing a test for the case shown in the original issue: #75094 (comment). In that case the plural's count argument specifies a format so I believe the formatted count string should be used for substitutions in the plural.
0976fd9 to
a705d59
Compare
|
Actually, this PR doesn't change the behavior of the flutter/packages/flutter_tools/test/integration.shard/test_data/gen_l10n_project.dart Line 514 in b217575
The PR only handles the strings around it. On the other hand, it also adds the functionality of But there is no harm to change the new tests to match that, which was done. Another thing discovered in the last commit, we should escape the variable if the suffix can be merged with it, e.g. return 'ES - Oh, she found ${pluralString}ES - !';instead of return 'ES - Oh, she found $pluralStringES - !'; |
8b5a54b to
9a38656
Compare
Co-authored-by: Shi-Hao Hong <[email protected]>
Co-authored-by: Shi-Hao Hong <[email protected]>
797670d to
ffc2048
Compare
|
@asashour - @jonahwilliams provided some feedback on #85116 - #85116 (review) which may still apply here. |
Thanks for your feedback. If I understand correctly, there is a case in which If this is not enough, please put a comment/issue with a test case, and cc/assign me. |
|
Thanks @jonahwilliams for feedback, I also changed already existing tests for the uneeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is a nice improvement to the i18n system, thanks for sticking with it!
We'll need to update the i18n user's guide - flutter.dev/go/i18n-user-guide - to cover the new features.
|
@asashour - Per #86167 (comment), I changed "fixes" to "addresses some of the issues" when referring to If you're willing to tackle the rest of #75094, please assign yourself to it. I tried to do so just now; for some reason GitHub wouldn't allow it. |
|
Now I see the issue, it is whether I would work on it, should be delivered in 24 hours, either in this PR (if not merged), or another one. It seems that issues can not be assigned to non-team members. |
|
Wow, we certainly would appreciate that. I think a separate PR would be best, since this one is already queued up for a landing. |
|
@HansMuller any chance this PR can make it to the next (upcoming) stable version? |
|
The next stable release is a ways off, but yes it should be part of that. |
|
Since this is merged, you can raise another PR to discuss your changes. I guess the next big thing is supporting plurals inside selects and vice versa. I guess regular expressions wouldn't help much, and possibly a parser needs to be built, but I think this should be discussed with the team before putting efforts. |
|
@HansMuller a hot fix for the current releases are not planned? This is the recommended way for multi language in flutter and it would be helpful to support all tags there as fast as possible, because the Thank you @asashour for your effort to this pull request. |
|
@asashour Thanks for your answer. You nailed it: I had to replace regexes with mini-parser to implement my POC for multiple/nested select and plurals. This change is not trivial and I will open a ticket to discuss it, now that your PR is merged. Thanks again for your work. ;) |
|
@HansMuller this would help to verify that it now works correctly with our use cases. But we are not going to a beta release for our product so we need to wait until it is part of a stable release. |
This is based on top of #85116 (since the expectations build on each other).
Addresses some of the issues raised in #75094
Fixes #81981
It supports
pluralsandselectwhen they are inside a text.Please note that the previous test case for
helloWorldsInterpolationforESwas wrong, since we didn't process the prefix string, but now we do. This is also fixed in this PR.Pre-launch Checklist
///).