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-11874 Test to verify Subdivisions vs Common #4349

Merged
merged 3 commits into from
Feb 10, 2025

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Feb 6, 2025

CLDR-11874

  • test to verify common/subdivisions vs common/main

  • tool to copy from common/main to common/subdivisions

  • logKnownIssue on CLDR-18296 for any data in common/subdivisions missing from common/main

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

@macchiati
Copy link
Member

The common/main is primary, so the json generation code should just skip paths in a common/subdivisions locale that have values in the corresponding common/main locale.

@srl295
Copy link
Member Author

srl295 commented Feb 7, 2025 via email

@macchiati
Copy link
Member

that would work. I commented yesterday in response to a slack question. Copying here:

Ah, now I see what the problem is. There is a 4th choice, which is to read from 2 CLDRFiles per locale. Get the merged list of locales. For each locale, load the subdivision/ data into a map<path, value>. Then load on top of that the main/ values filtered to subdivision paths. Then write out the map (if non-empty). The downside is that it uses different loading logic than the normal. (edited)

However, I'm ok with (C); add a CLDRModify step to copy from main/ to subdivisions/. Have a unit test that you run before json generation to verify

New tool, CopyMainToSubdivisions - run it after other CLDRModify passes but before JSON generation
New test in TestSubdivisions, with a logknownissue on CLDR-18296 for data that is missing from common/main
- update the approval status of items
- copy items from common/main into subdivisions
@srl295 srl295 marked this pull request as ready for review February 7, 2025 20:42
@@ -421,7 +421,7 @@ CLDR data files are interpreted according to the LDML specification (http://unic
<subdivision type="gbdur" draft="provisional">Durham-graafskap</subdivision>
<subdivision type="gbedh" draft="provisional">Edinburg</subdivision>
<subdivision type="gbels" draft="provisional">Buite-Hebride</subdivision>
<subdivision type="gbeng" draft="provisional">Engeland</subdivision>
<subdivision type="gbeng">Engeland</subdivision>
Copy link
Member Author

Choose a reason for hiding this comment

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

approval status copied from common/main (via survey) into subdivisions

@@ -1615,7 +1615,7 @@ CLDR data files are interpreted according to the LDML specification (http://unic
<subdivision type="gbwkf" draft="provisional">cité de Wakefield</subdivision>
<subdivision type="gbwll" draft="provisional">district métropolitain de Walsall</subdivision>
<subdivision type="gbwln" draft="provisional">West Lothian</subdivision>
<subdivision type="gbwls" draft="provisional">pays de Galles</subdivision>
<subdivision type="gbwls">Pays de Galles</subdivision>
Copy link
Member Author

Choose a reason for hiding this comment

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

an actual change (and approval) from common/main

Comment on lines +15 to +17
<subdivision type="gbeng">Ĩgratéra</subdivision>
<subdivision type="gbsct">Enhkósija</subdivision>
<subdivision type="gbwls">Vares</subdivision>
Copy link
Member Author

Choose a reason for hiding this comment

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

data in a new locale

@srl295
Copy link
Member Author

srl295 commented Feb 7, 2025

with this change cldr json generation works properly.

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.

Very good.

One request: could you file a ticket to make CLDRFile.make be thread-safe, to avoid

                                synchronized (CLDRFile.class) {
                                    mainF = mainFactory.make(loc, true);
                                    subF = subFactory.make(par.getBaseName(), true);
                                }

I'm sure there are other places in the code that aren't as careful as you are!

@srl295 srl295 merged commit 8267b08 into unicode-org:main Feb 10, 2025
12 checks passed
@srl295 srl295 deleted the cldr-11874/verify-subdiv-equiv branch February 10, 2025 14:48
@srl295
Copy link
Member Author

srl295 commented Feb 10, 2025

Very good.

One request: could you file a ticket to make CLDRFile.make be thread-safe, to avoid

                                synchronized (CLDRFile.class) {
                                    mainF = mainFactory.make(loc, true);
                                    subF = subFactory.make(par.getBaseName(), true);
                                }

I'm sure there are other places in the code that aren't as careful as you are!

Couldn't reproduce the issue now, i thought I had put a stack trace somewhere but can't find it.

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