CLDR-18722 Explicit numberSystem attribute #4974
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 count="other" draft="contributed">pesetas</displayName> | ||
| <symbol>₧</symbol> | ||
| <decimal>,</decimal> | ||
| <group>❰NBTSP❱</group> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@macchiati Did you file that ticket yet?
| <!ATTLIST decimal draft (approved | contributed | provisional | unconfirmed | true | false) #IMPLIED > | ||
| <!--@METADATA--> | ||
| <!--@DEPRECATED:true, false--> | ||
| <!ATTLIST decimal references CDATA #IMPLIED > |
There was a problem hiding this comment.
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.
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.
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.
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.
@srl295 I forget if we discussed this in the infrastructure meeting or slack. Although it's tangential to this ticket, maybe there should be a new ticket for it?
There was a problem hiding this comment.
No it's already on my list, just ignore that file i have an issue to correct it
There was a problem hiding this comment.
i recommend to revert changes to this file
| NumericType type = NumericType.getNumericType(path); | ||
| if (type == NumericType.NOT_NUMERIC || type == NumericType.RATIONAL) { | ||
| return this; // skip | ||
| } |
There was a problem hiding this comment.
I take it that NumericType still gets these items, so they aren't skipped by line 258.
There was a problem hiding this comment.
Sorry for the late response; this PR was put on hold pending branching maint/maint-48.
I don't quite understand the question.
This is more than 100 lines into handleCheck and this PR only does two things here, a few lines further down:
- It re-enables the "Number formats must have an explicit numberSystem attribute." check.
- It makes the check more general, since the
numberSystemattribute can occur in different places, not necessary index 2.
Is the concern that some paths that would now match would be skipped prematurely because they have NumericType.NOT_NUMERIC or NumericType.RATIONAL? I could investigate that...
|
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
|
Now that we've branched maint/maint-48, I've made this ready for review again, and added Steven and Peter as reviewers, and responding to a couple of comments |
|
I see the ticket is now done, with this follow-up ticket for v49: https://unicode-org.atlassian.net/browse/CLDR-18963 I don't know if there's a way to move the PR to a different ticket; should I just make a new PR and refer back to this one for discussion? |
Just retitle the pr and then either use the auto squash link or manually reword the git commits and force push |
| <!--@METADATA--> | ||
| <!ATTLIST group numberSystem CDATA #IMPLIED > | ||
| <!--@DEPRECATED--> | ||
| <!--@MATCH:bcp47/nu--> |
There was a problem hiding this comment.
| <!--@MATCH:bcp47/nu--> | |
| <!--@DEPRECATED--> |
There was a problem hiding this comment.
please revert all changes to the test file. i'll document it better in https://unicode-org.atlassian.net/browse/CLDR-19058
| <!--@METADATA--> | ||
| <!ATTLIST decimal numberSystem CDATA #IMPLIED > | ||
| <!--@DEPRECATED--> | ||
| <!--@MATCH:bcp47/nu--> |
There was a problem hiding this comment.
| <!--@MATCH:bcp47/nu--> | |
| <!--@DEPRECATED--> |
-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