-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Roll pub packages manually #145170
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
Roll pub packages manually #145170
Conversation
| expect(localizations.selectedRowCountTitle(10000), 'បានជ្រើសរើសធាតុ 10.000'); | ||
| expect(localizations.selectedRowCountTitle(123456789), 'បានជ្រើសរើសធាតុ 123.456.789'); | ||
| expect(localizations.selectedRowCountTitle(10000), 'បានជ្រើសរើសធាតុ 10,000'); | ||
| expect(localizations.selectedRowCountTitle(123456789), 'បានជ្រើសរើសធាតុ 123,456,789'); |
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.
@goderbauer and I did some research:
- Wikipedia article indicates Khmer uses commas as the thousands separator: https://en.wikipedia.org/wiki/Khmer_numerals#Numbers_from_100_to_10,000,000
- Google Translate uses commas as the thousands separator: https://translate.google.com/?sl=en&tl=km&text=I%20have%201%2C000%20apples&op=translate
- Gemini agrees it should be a comma:
The Khmer language uses a comma (,) as its thousands separator. However, you might also see spaces used as a thousands separator, especially in less formal contexts.
EDIT: Copying this comment: #145167 (comment)
Just FYI: The authority here is the Unicode CLDR data, which agrees on the grouping separator being a comma.
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.
(Drive-by reviewing, feel free to ignore) ICU says:
Numbers
- Grouping separator:
.- Decimal separator:
,
Sauce:
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.
Sometimes software is too hard
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.
Looks like this change is correct, per: #145167 (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.
Hah, and people complain about timezones :P What an amazing "Winning" column in that link :P
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 is where the separators flipped:
(Looks like ICU4c hasn't regenerated data from the latest CLDR yet?)
goderbauer
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.
LGTM
Thank you!
christopherfujino
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.
ship it
eliasyishak
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.
I need to make an update to analytics for 5.8.7, can we wait until that publishes in a few minutes?
|
auto label is removed for flutter/flutter/145170, due to This PR has not met approval requirements for merging. Changes were requested by {eliasyishak}, please make the needed changes and resubmit this PR.
|
|
@eliasyishak No problem. Let me know when you're good to go! |
|
For context, Elias and I believe that if we merge this as is, the version of unified_analytics here will make dart-lang/tools#252 even worse, so we're going to try to roll this forward two hotfixes. |
The automated roll failed as a test needs to be updated: #145167
Fixes: #139861
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.