-
Notifications
You must be signed in to change notification settings - Fork 384
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-17535 BRS Build language containment #3980
CLDR-17535 BRS Build language containment #3980
Conversation
Tests pass now, so I'd appreciate a review. |
a local run of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still looking a the spreadsheet but I'm going to say LGTM for now
@@ -1,14 +1,18 @@ | |||
package org.unicode.cldr.util; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package org.unicode.cldr.util; | |
package org.unicode.cldr.tool; |
If this is really a tool, it should go under the tool package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I just didn't want to do that in a single PR.
@@ -603,67 +711,67 @@ private static Set<Set<String>> getAncestors(String leaf, Set<String> skipping) | |||
} | |||
return itemsFixed; | |||
// TODO: delete this commented-out code? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete it instead of fixing its indent?
Chart is helpful. This is a little concerning:
any reason why these should be missing? |
But that doesn't let you know what all the codes actually mean, and when they switch from one parent to another (or are just removed). |
The diffs are in the spreadsheet. https://docs.google.com/spreadsheets/d/1vx3qzX_gO_FJmNl9NgcC-_z2D4Rxp02WgylnEINQdXc/edit?gid=653556033#gid=653556033 I didn't focus on the OC cases, because they haven't reached basic; once they do, then this is worth following up on; but by that time, Wikidata might have data.
FYI: the removal of Ω doesn't mean much; either way, there isn't a known parent. |
Ok I misread I thought it has a parent before. |
CLDR-17535
The output is impossible to diff, so the DiffLanguageGroups.java is needed to make sense of the differences.
I modified DiffLanguageGroups to make it easier to compare the changes. The current results are in https://docs.google.com/spreadsheets/d/1vx3qzX_gO_FJmNl9NgcC-_z2D4Rxp02WgylnEINQdXc/edit?gid=653556033 (the Containment sheet).
It is highlighting the changes in the TC locales and Other Basic+ locales, to focus the review on them, although all of the differences are listed. I addressed the most concerning ones, by adding overrides (also cleaned up the code around that.
The DiffLanguageGroups is also automatically run at the end of the generation.
ALLOW_MANY_COMMITS=true