Skip to content

Conversation

@loic-sharma
Copy link
Member

@loic-sharma loic-sharma commented Mar 14, 2024

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.

@loic-sharma loic-sharma requested a review from goderbauer March 14, 2024 18:25
@github-actions github-actions bot added tool Affects the "flutter" command-line tool. See also t: labels. framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. a: internationalization Supporting other languages or locales. (aka i18n) d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Mar 14, 2024
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');
Copy link
Member Author

@loic-sharma loic-sharma Mar 14, 2024

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:

  1. Wikipedia article indicates Khmer uses commas as the thousands separator: https://en.wikipedia.org/wiki/Khmer_numerals#Numbers_from_100_to_10,000,000
  2. Google Translate uses commas as the thousands separator: https://translate.google.com/?sl=en&tl=km&text=I%20have%201%2C000%20apples&op=translate
  3. 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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

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

Copy link
Contributor

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)

Copy link
Member

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

Copy link
Member

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:

https://unicode.org/cldr/charts/44/delta/km.html

(Looks like ICU4c hasn't regenerated data from the latest CLDR yet?)

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you!

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

ship it

@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 14, 2024
Copy link
Contributor

@eliasyishak eliasyishak left a 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-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 14, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Mar 14, 2024

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.
The PR author is a member of flutter-hackers and needs 0 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@loic-sharma
Copy link
Member Author

@eliasyishak No problem. Let me know when you're good to go!

@christopherfujino
Copy link
Contributor

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.

@eliasyishak eliasyishak dismissed their stale review March 15, 2024 13:35

Updated package

@eliasyishak eliasyishak added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 15, 2024
@auto-submit auto-submit bot merged commit 8d54abf into flutter:master Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
@loic-sharma loic-sharma deleted the intl_manual_roll branch March 15, 2024 16:39
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: internationalization Supporting other languages or locales. (aka i18n) autosubmit Merge PR when tree becomes green via auto submit App d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Version solving failed for intl: 0.19.0 with flutter_localizations

5 participants