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-23033 Regenerate scx value array #3355

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Jan 28, 2025

This fixes the scx array hardcoded in icuexportdata for Unicode 16 (see: unicode-org/icu4x#6041)

This is a quick and dirty fix, I've tried to include the python script I used to generate this. We should still fix ICU-21821.

Checklist

  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-23033
  • Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • 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. Example: "ICU-1234 Fix xyz"
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@Manishearth
Copy link
Member Author

r? @markusicu @echeran @sffc

@Manishearth
Copy link
Member Author

I haven't tested the output, please do check that the codepoints are the right ones.

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/tools/icuexportdata/icuexportdata.cpp 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!

  • icu4c/source/tools/icuexportdata/icuexportdata.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Copy link
Contributor

@FrankYFTang FrankYFTang left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@sffc
Copy link
Member

sffc commented Jan 30, 2025

This PR does not fix ICU-21821 and therefore should not be tagged with that issue number.

Is there a rush for this? I would rather see the actual fix. There's no good way to review this PR and we rely on the astute observations of people like @FrankYFTang to catch issues.

@Manishearth
Copy link
Member Author

@sffc we have clients reporting discrepancies in their data because of this. The correct fix involves adding APIs to ICU4C, unless that can be done in a short period of time, I think we should land this and get a new icuexportdata. Or we should somehow patch this data into 2.0.0-beta1 datagen: the goal is to get updated data released, it's not important if icuexportdata is fixed in the meantime.

I'm happy to rewrite the Python code to be a bit more readable and reviewable.

@markusicu markusicu self-assigned this Jan 30, 2025
@markusicu markusicu requested a review from echeran January 30, 2025 17:26
@markusicu
Copy link
Member

@sffc points out that this PR does not fix what ICU-21821 says, so we need a different ICU ticket for this quick fix and update the PR description and the commit message.

Copy link
Contributor

@FrankYFTang FrankYFTang left a comment

Choose a reason for hiding this comment

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

Agree w/ @sffc, please file a new ticket and change the PR to use that new ticket

@Manishearth Manishearth changed the title ICU-21821 Regenerate scx value array ICU-23033 Regenerate scx value array Jan 31, 2025
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/tools/icuexportdata/icuexportdata.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Copy link
Contributor

@FrankYFTang FrankYFTang left a comment

Choose a reason for hiding this comment

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

LGTM

@markusicu markusicu merged commit bf675e3 into unicode-org:main Jan 31, 2025
94 checks passed
@Manishearth
Copy link
Member Author

@Manishearth Manishearth deleted the scx-regenerate branch February 1, 2025 06:54
@Manishearth
Copy link
Member Author

@sffc can you do the uplift dance to get a tagged release? I forgot what needs to be done, is it just a cherry-pick PR to maint?

@sffc
Copy link
Member

sffc commented Feb 2, 2025

I think we should just tag from the main branch. It has CLDR 47, but we should be integrating CLDR 47 into ICU4X within the next week anyway.

CC @echeran who usually does 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