Skip to content

Conversation

@conradarcturus
Copy link
Contributor

@conradarcturus conradarcturus commented Jan 27, 2025

These values were added to CLDR in between submission cycles and it would be unfair to drop locale coverage if the languages did not support them. I'm not happy with the styling of all of my code here -- I'm happy to respond to suggestions.

Thereby, we're temporarily overriding their coverage as "comprehensive". All other coverage level changes are for other reasons (and Haitian Creole has a separate ticket to fix it). You can see changes to charts in this PR unicode-org/cldr-staging#168 Some changes to charts are from other PRs, but the ones keeping coverage level at a good level are there.

While I was here I also had put in the fix for the ticket on Haitian Creole (https://unicode-org.atlassian.net/browse/CLDR-18105) -- because tests are failing since it thought Haitian Creole is a modern locale and thereby it should have translations in other languages (which we don't have yet because its new). So commit #3 does that change.

CLDR-18258

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

These values were added to CLDR in between submission cycles and it would be unfair to drop locale coverage if the languages did not support them.

Thereby, we're temporarily overriding their coverage as "comprehensive".
@conradarcturus conradarcturus marked this pull request as ready for review January 29, 2025 09:19
@conradarcturus conradarcturus requested review from macchiati, pedberg-icu and srl295 and removed request for macchiati, pedberg-icu and srl295 January 29, 2025 09:19
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.

Looks pretty good, however to appears to go overboard with the script-specific currency data. We should only be targeting one currency, after all, and that should be done with the variables.

Also, tests fail.

Side note: 'fair' isn't the right term.

@conradarcturus
Copy link
Contributor Author

Thanks for taking a look. It takes 10 minutes to run all of the code so its been a long game of change and hope the result passes every test. Apologies there still seems to be errors. I'll do another pass to try to resolve them. Let me know if you have tips to make running tests faster.

The script specific currency changes were solved what I thought you were asking for, In the original email you indicated we needed to change these to comprehensive:

All the ZWG currency elements
Currency alphaNextToNumber, noCurrency
Short Currency using XXX Digits
light-speed

The script-specific changes are for Currency alphaNextToNumber, noCurrency, and Short currency -- since that problem was fixed in Latn but introduced for non-latin scripts.

@conradarcturus conradarcturus force-pushed the CLDR-18258-exclude-new-entries-from-coverage branch from 0f1d7ff to d7f83fe Compare January 31, 2025 01:34
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-code/src/main/java/org/unicode/cldr/util/StandardCodes.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

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 was worried that you were including too much, But I reviewed it in detail (once I had more than a phone!), and it looks good. Since you are off in different timezones, I'm going to go ahead and merge.

@macchiati macchiati merged commit 3793325 into main Jan 31, 2025
16 checks passed
@conradarcturus conradarcturus deleted the CLDR-18258-exclude-new-entries-from-coverage branch January 31, 2025 03:01
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.

3 participants