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

CLDR-18133 supplemental currency needs to be @ORDERED #4344

Merged
merged 2 commits into from
Feb 6, 2025

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Feb 5, 2025

CLDR-18133

  • also fix a SPEC typo- @ORDERED applies to this element, not to children.

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

@srl295 srl295 self-assigned this Feb 5, 2025
@srl295
Copy link
Member Author

srl295 commented Feb 5, 2025

This doesn't fix the JSON issue, but it was discovered while working on it…

@srl295
Copy link
Member Author

srl295 commented Feb 5, 2025

Something's wrong, because CLDRFile with a DTDComparator is returning lexical instead of file order.

//supplementalData/currencyData/region[@iso3166="BY"]/currency[@iso4217="BYB"][@from="1994-08-01"][@to="2000-12-31"]
//supplementalData/currencyData/region[@iso3166="BY"]/currency[@iso4217="BYN"][@from="2016-07-01"]
//supplementalData/currencyData/region[@iso3166="BY"]/currency[@iso4217="BYR"][@from="2000-01-01"][@to="2017-01-01"]
//supplementalData/currencyData/region[@iso3166="BY"]/currency[@iso4217="RUR"][@from="1991-12-25"][@to="1994-11-08"]
//supplementalData/currencyData/region[@iso3166="BY"]/currency[@iso4217="SUR"][@from="1961-01-01"][@to="1991-12-25"]

Should be:

  • BYN
  • BYR
  • BYB
  • RUR
  • SUR

- add a test for currency
- also, fix typo in tr35.md
@srl295 srl295 force-pushed the cldr-18133/json-currency-order branch from 4af4ba8 to 91fe975 Compare February 5, 2025 22:07
@srl295 srl295 marked this pull request as ready for review February 5, 2025 22:07
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • common/dtd/ldmlSupplemental.dtd is different
  • docs/ldml/tr35.md is now changed in the branch
  • tools/cldr-code/src/test/java/org/unicode/cldr/util/TestCLDRFile.java is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@srl295
Copy link
Member Author

srl295 commented Feb 5, 2025

OK. I fixed the DTD and added a test.

I'll fix the JSON in a followon PR.

macchiati
macchiati previously approved these changes Feb 5, 2025
@srl295 srl295 merged commit d0f6560 into unicode-org:main Feb 6, 2025
12 checks passed
@srl295 srl295 deleted the cldr-18133/json-currency-order branch February 6, 2025 15:14
@srl295
Copy link
Member Author

srl295 commented Feb 6, 2025

FYI this did fix the JSON data- but only when the dtd is also copied into cldr-staging, since the json converter uses CLDR_DIR of cldr-staging.

@macchiati
Copy link
Member

macchiati commented Feb 6, 2025 via email

@srl295
Copy link
Member Author

srl295 commented Feb 6, 2025

Good! Using the cldr staging version seems fragile, unless there is a test that it matches head of cldr

It's actually correct here. Because the CLDR_DIR is pointing at, say, release-47-beta3 etc. So cldr staging is the correct place for the DTD.

My problem is that I was changing the CLDR tooling at the same time. If I was patching the data, I would have had to run GenerateProductionData for example.

@macchiati
Copy link
Member

macchiati commented Feb 7, 2025 via email

@srl295
Copy link
Member Author

srl295 commented Feb 10, 2025

Makes sense. I'm just wondering how we can make sure that other people don't stumble over this problem in the future.

automating GenerateProductionData is probably the best way.

@macchiati
Copy link
Member

macchiati commented Feb 11, 2025 via email

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.

2 participants