Skip to content
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

ICU-21519 Make compact notation 'c' behave like 'e' in FixedDecimal #1624

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

echeran
Copy link
Contributor

@echeran echeran commented Mar 7, 2021

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-21519
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/PluralRulesTest.java is different
  • icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/DecimalQuantityTest.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4j/main/classes/core/src/com/ibm/icu/text/PluralRules.java is different
  • icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/PluralRulesTest.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@echeran echeran marked this pull request as ready for review March 8, 2021 01:09
@echeran echeran requested review from sffc, markusicu and macchiati March 8, 2021 01:09
@markusicu markusicu requested a review from pedberg-icu March 8, 2021 23:19
Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

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

rslgtm

I know we discussed need this for CLDR 39 to include this data.

The changes look plausible. I would prefer if someone who has been in on these discussion could review the changes, but I don't want you (& CLDR) to be blocked.

Copy link
Member

@macchiati macchiati left a comment

Choose a reason for hiding this comment

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

I think this works; it probably does a bit more work than it needs to, since I think it could have just mapped 'c' to 'e' on input, and 'e' to 'c' on output, rather than supporting both. But that can always be adjusted later, since this is not public api.
cc @sffc

@pedberg-icu
Copy link
Contributor

OK to merge this so I can pick up an ICU with this fix for CLDR?

@pedberg-icu
Copy link
Contributor

The appveyor failure is a segfault while compiling translit.cpp, certainly nothing to do with this change or (likely) even ICU. I am going to go ahead and merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants