-
Notifications
You must be signed in to change notification settings - Fork 400
CLDR-18520 Update Kosovo subdivisions in en.xml #4613
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
base: main
Are you sure you want to change the base?
Conversation
Update Kosovo subdivisions in en.xml to reflect latest administrative divisions
Generally LGTM, however... Since Kosovo (XK) is not universally recognized, we may want to leave the Serbian Kosovo regions in place. I’m not certain what the precedent is. I'm tagging other people to review this change. |
A better name for this PR would be "Update Kosovo subdivisions in en.xml" |
Thank you for the feedback. I’ve retained the old subdivisions as they were, while also updating the name as requested. I understand the complexity regarding the recognition of Kosovo, and I’ve ensured that the previous data is still present. I’ve also updated the PR title to "Update Kosovo subdivisions in en.xml" for clarity. |
Looks good to me but I want someone else from CLDR to accept since they have more experience with changes like this. |
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.
Two tests are failing:
TestAttributeValues {
TestValid
Warning: (TestAttributeValues.java:301) Warning: Invalid /home/runner/work/cldr/cldr/common/subdivisions/en.xml subdivision type xk01
...
TestPaths {
TestNonLdml
Error: (TestPaths.java:630) Error: subdivisions/en.xml/subdivision@type, expected match to: ⟪or/validity/subdivision||literal/AS, AW, AX, BL, CP, CW, GF, GP, GU, HK, IC, MF, MO, MP, MQ, NC, PF, PM, PR, RE, SX, TA, TF, TW, UM, VI, WF, YT, itsd, no50⟫ actual: «xk01»
and similarly for xk02, ... .
I'm not familiar with that code. From a quick look it I suspect both tests expect to find these values enumerated in the DTD as allowable values and it's not finding them there.
<subdivision type="rs27">Prizren</subdivision> <!-- in rskm : Kosovo-Metohija --> | ||
<subdivision type="rs28">Kosovska Mitrovica</subdivision> <!-- in rskm : Kosovo-Metohija --> | ||
<subdivision type="rs29">Kosovo-Pomoravlje</subdivision> <!-- in rskm : Kosovo-Metohija --> | ||
<subdivision type="rskm">Kosovo-Metohija</subdivision> |
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.
It looks like a space has been inserted before the first tab in each of these lines, otherwise there's no change in these lines
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.
ideally revert that
the ISO data set won't contain Kosovar subdivisions I assume. Probably needs more discussion. |
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.
This can't be done. See https://unicode-org.atlassian.net/browse/CLDR-18520?focusedCommentId=182868
Update Kosovo subdivisions in en.xml to reflect latest administrative divisions
CLDR-18520
ALLOW_MANY_COMMITS=true