-
Notifications
You must be signed in to change notification settings - Fork 401
CLDR-18722 Explicit numberSystem attribute #4974
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
-Revise ldml.dtd to allow numberSystem attribute for pattern element -Add numberSystem (latn) for pattern/decimal/group elements where needed in locales ca, el, en_150, en_AT, eu, gl, it, tr -Re-enable the check for numberSystem in CheckNumbers.java -Use parts.containsAttribute instead of parts.getAttribute(2, ...) since the element index may not be 2
Locally, after making these changes, I ran console check as follows:
Note that |
-Remove numberSystem from a path where it should not have been added (redundant)
-Revise ldml.dtd to handle numberSystem attribute for decimal/group element like elsewhere, not deprecated
<displayName draft="contributed">Peseta española</displayName> | ||
<displayName count="one" draft="contributed">peseta</displayName> | ||
<displayName count="other" draft="contributed">pesetas</displayName> | ||
<symbol>₧</symbol> | ||
<decimal>,</decimal> | ||
<group>❰NBTSP❱</group> |
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.
There is something wrong here; unrelated to this ticket. The [❰❱] characters should never occur in values for our data files. It looks like vetters might be pasting them into values. I'll file a separate ticket about this.
@@ -2051,7 +2051,7 @@ CLDR data files are interpreted according to the LDML specification (http://unic | |||
<!ATTLIST decimal references CDATA #IMPLIED > |
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.
I'm wondering why this file exists; I've noticed that it causes errors when it drifts way from the regular ldml.dtd. If we keep it, we should have a test that it is identical (and maybe we do, and that's what alerted you to it?)
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.
In eclipse I see the following error with that file:
Description Resource Path Location Type
The element 'gmtUnknownFormat' has not been declared. ldml.dtd /cldr-code/src/test/resources/org/unicode/cldr/unittest/data/common/dtd line 1723 DTD Problem
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.
I'm confused by that too. There are 3 ldml.dtd files:
- common/dtd/ldml.dtd
- tools/cldr-code/src/test/resources/org/unicode/cldr/unittest/data/common/dtd/ldml.dtd
- tools/cldr-code/target/test-classes/org/unicode/cldr/unittest/data/common/dtd/ldml.dtd
I was accidentally editing the wrong one before I noticed that...
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.
In fact I did that again even after realizing there are 3 files; this PR still fails maybe for that reason. I'll make another (4th) commit shortly.
@srl295 do you have insight about how the ldml.dtd files relate to each other?
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.
Looking good; a couple of questions
@@ -259,12 +259,8 @@ public CheckCLDR handleCheck( | |||
} |
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.
I take it that NumericType still gets these items, so they aren't skipped by line 258.
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.
I was thinking about this, and it is really past when we can make changes that could affect ICU. So we might want to put this on hold until we branch. Let's discuss in infra.
Changing to Draft so we don't mistakenly merge into 48 main. |
-A previous commit mistakenly did this with the wrong ldml.dtd; there are 2 files with that name, not counting the one in target
-Revise ldml.dtd to allow numberSystem attribute for pattern element
-Add numberSystem (latn) for pattern/decimal/group elements where needed in locales ca, el, en_150, en_AT, eu, gl, it, tr
-Re-enable the check for numberSystem in CheckNumbers.java
-Use parts.containsAttribute instead of parts.getAttribute(2, ...) since the element index may not be 2
CLDR-18722
ALLOW_MANY_COMMITS=true