CLDR-18963 Allow numberSystem attribute for currency element #5205

btangmu · 2025-11-25T18:08:56Z

-Revise ldml.dtd to allow numberSystem attribute for currency element

-Delete the (disabled) check for numberSystem in CheckNumbers.java

-Update attributeOrderGrandfathered, for currency, allow numberSystem after references

This PR completes the ticket.

ALLOW_MANY_COMMITS=true

-Revise ldml.dtd to allow numberSystem attribute for currency element -Delete the (disabled) check for numberSystem in CheckNumbers.java

-Move numberSystem after draft

-Move numberSystem after references and validSubLocales

btangmu · 2025-11-25T20:20:22Z

I think this PR is between essentially the same rock and hard place as the previous PR (#5149). Here we're adding numberSystem for currency not for pattern, but otherwise it's the same problem.

Per testAttributeOrder status (distinguished before value before metadata), we need numberSystem (distinguished) before draft (metadata). See:

public enum AttributeStatus {
    distinguished("§d"),
    value("§v"),
    metadata("§m︎");

But, per MergeLists, we need draft before numberSystem for consistency with, e.g.,

<!ELEMENT decimal ( #PCDATA ) >
<!ATTLIST decimal alt NMTOKENS #IMPLIED >
    <!--@MATCH:literal/variant, us, official-->
<!ATTLIST decimal draft (approved | contributed | provisional | unconfirmed | true | false) #IMPLIED >
    <!--@METADATA-->
    <!--@DEPRECATED:true, false-->
<!ATTLIST decimal references CDATA #IMPLIED >
    <!--@METADATA-->
<!ATTLIST decimal numberSystem CDATA #IMPLIED >
    <!--@MATCH:bcp47/nu-->

A most ingenious paradox.

macchiati · 2025-11-26T16:41:18Z

A refinement of my previous advice.

The order is vital among distinguishing attributes (no @metadata and no @value) and @value attributes, but not with @metadata attributes. So you can move the @metadata items (keeping their same relative order) to the end of the items.

macchiati · 2025-11-26T16:43:10Z

It is still good that we tripped across this in the first PR, because having (optional) numberSystem attributes for currency elements is the better structure.

btangmu · 2025-11-26T22:14:54Z

move the @metadata items (keeping their same relative order) to the end of the items

I've just tried that locally. Starting with

<!ATTLIST currency type NMTOKEN "standard" >
    <!--@MATCH:validity/currency-->
<!ATTLIST currency alt NMTOKENS #IMPLIED >
    <!--@MATCH:literal/variant-->
<!ATTLIST currency draft (approved | contributed | provisional | unconfirmed | true | false) #IMPLIED >
    <!--@METADATA-->
    <!--@DEPRECATED-->
<!ATTLIST currency references CDATA #IMPLIED >
    <!--@METADATA-->
<!ATTLIST currency validSubLocales CDATA #IMPLIED >
    <!--@VALUE-->
    <!--@DEPRECATED-->

The item to be added is

<!ATTLIST currency numberSystem CDATA #IMPLIED >
    <!--@MATCH:bcp47/nu-->

The two metadata items to be moved are draft and references. So I have tried this:

<!ATTLIST currency type NMTOKEN "standard" >
    <!--@MATCH:validity/currency-->
<!ATTLIST currency alt NMTOKENS #IMPLIED >
    <!--@MATCH:literal/variant-->
<!ATTLIST currency validSubLocales CDATA #IMPLIED >
    <!--@VALUE-->
    <!--@DEPRECATED-->
<!ATTLIST currency numberSystem CDATA #IMPLIED >
    <!--@MATCH:bcp47/nu-->
<!ATTLIST currency draft (approved | contributed | provisional | unconfirmed | true | false) #IMPLIED >
    <!--@METADATA-->
    <!--@DEPRECATED-->
<!ATTLIST currency references CDATA #IMPLIED >
    <!--@METADATA-->

The test result is:

java.lang.IllegalArgumentException: Inconsistent requested ordering: cannot merge if we have [...A...B...] and [...B...A...]: {references=[draft, references], standard=[draft, standard, references, validSubLocales], validSubLocales=[draft, standard, references, validSubLocales], numberSystem=[draft, validSubLocales, numberSystem], draft=[validSubLocales, numberSystem, draft, references]}
	at org.unicode.cldr.util.MergeLists.merge(MergeLists.java:68)

That is:

references     =[draft, references]
standard       =[draft, standard, references, validSubLocales]
validSubLocales=[draft, standard, references, validSubLocales]
numberSystem   =[draft, validSubLocales, numberSystem]
draft          =[validSubLocales, numberSystem, draft, references]

Meaning, I think, that MergeLists won't accept validSubLocales before draft since elsewhere there is validSubLocales after draft. Am I understanding your instructions and the result correctly?

It would help if the algorithm and error reporting of MergeLists were less mysterious. Would it be worthwhile to add some comments and debugging output to make it unambiguous what A and B are in "cannot merge if we have [...A...B...] and [...B...A...]"? Here it seems A and B could be validSubLocales and draft, or draft and numberSystem, or references and validSubLocales...

macchiati · 2025-11-27T16:59:21Z

You can leave validSubLocales where it was. Since it is depreciated, it doesn't matter.

…

On Wed, Nov 26, 2025, 14:15 Tom Bishop ***@***.***> wrote: *btangmu* left a comment (unicode-org/cldr#5205) <#5205 (comment)> move the @metadata <https://github.com/metadata> items (keeping their same relative order) to the end of the items I've just tried that locally. Starting with <!ATTLIST currency type NMTOKEN "standard" > ***@***.***:validity/currency--> <!ATTLIST currency alt NMTOKENS #IMPLIED > ***@***.***:literal/variant--> <!ATTLIST currency draft (approved | contributed | provisional | unconfirmed | true | false) #IMPLIED > ***@***.***> ***@***.***> <!ATTLIST currency references CDATA #IMPLIED > ***@***.***> <!ATTLIST currency validSubLocales CDATA #IMPLIED > ***@***.***> ***@***.***> The item to be added is <!ATTLIST currency numberSystem CDATA #IMPLIED > ***@***.***:bcp47/nu--> The two metadata items to be moved are draft and references. So I have tried this: <!ATTLIST currency type NMTOKEN "standard" > ***@***.***:validity/currency--> <!ATTLIST currency alt NMTOKENS #IMPLIED > ***@***.***:literal/variant--> <!ATTLIST currency validSubLocales CDATA #IMPLIED > ***@***.***> ***@***.***> <!ATTLIST currency numberSystem CDATA #IMPLIED > ***@***.***:bcp47/nu--> <!ATTLIST currency draft (approved | contributed | provisional | unconfirmed | true | false) #IMPLIED > ***@***.***> ***@***.***> <!ATTLIST currency references CDATA #IMPLIED > ***@***.***> The test result is: java.lang.IllegalArgumentException: Inconsistent requested ordering: cannot merge if we have [...A...B...] and [...B...A...]: {references=[draft, references], standard=[draft, standard, references, validSubLocales], validSubLocales=[draft, standard, references, validSubLocales], numberSystem=[draft, validSubLocales, numberSystem], draft=[validSubLocales, numberSystem, draft, references]} at org.unicode.cldr.util.MergeLists.merge(MergeLists.java:68) That is: references =[draft, references] standard =[draft, standard, references, validSubLocales] validSubLocales=[draft, standard, references, validSubLocales] numberSystem =[draft, validSubLocales, numberSystem] draft =[validSubLocales, numberSystem, draft, references] Meaning, I think, that MergeLists won't accept validSubLocales before draft since elsewhere there is validSubLocales after draft. Am I understanding your instructions and the result correctly? It would help if the algorithm and error reporting of MergeLists were less mysterious. Would it be worthwhile to add some comments and debugging output to make it unambiguous what A and B are in "cannot merge if we have [...A...B...] and [...B...A...]"? Here it seems A and B could be validSubLocales and draft, or draft and numberSystem, or references and validSubLocales... — Reply to this email directly, view it on GitHub <#5205 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMDRR2UWSXNRUT6IN2T36YQ7HAVCNFSM6AAAAACNFQW2SKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKOBTGM4DQOBYGE> . You are receiving this because your review was requested.Message ID: ***@***.***>

btangmu · 2025-11-28T14:31:12Z

leave validSubLocales where it was

Then I still get:

Caused by: java.lang.IllegalArgumentException: Inconsistent requested ordering: cannot merge if we have [...A...B...] and [...B...A...]: {references=[draft, references], standard=[draft, standard, references, validSubLocales], validSubLocales=[draft, standard, references, validSubLocales], numberSystem=[draft, validSubLocales, numberSystem], draft=[numberSystem, draft, references, validSubLocales]}
	at org.unicode.cldr.util.MergeLists.merge(MergeLists.java:68)

That is,

references     =[draft, references]
standard       =[draft, standard, references, validSubLocales]
validSubLocales=[draft, standard, references, validSubLocales]
numberSystem   =[draft, validSubLocales, numberSystem]
draft          =[numberSystem, draft, references, validSubLocales]

The position of validSubLocales is different in the last part of the error message but the essential problem remains for other A, B versus B, A.

macchiati · 2025-12-01T22:30:34Z

The ordering of attributes within elements is controlled by a global MapComparator across all elements with a given DTD, to save having to have individual attribute comparators for each element. That MapComparator created using MergeLists, which merges the attributes based on their ordering in the DTD file. MergeLists also enforces that there are no cycles among elements; for example, you can’t have:

element1: attributeA < attributeB
element2: attributeB < attributeC
element3: attributeC < attributeA

The order of attributes should always be

Distinguishing
Value
Metadata

There is a test for that, but for backwards compatibility there is an exception list. So we retain the ‘bad’ order if we have to (by updating the exception list)

I took a look, and numberSystem is almost always after draft and references. So for currency let's move it before validSubLocales, and adjust the exception list in the failing test.

btangmu · 2025-12-02T15:53:57Z

numberSystem is almost always after draft and references. So for currency let's move it before validSubLocales, and adjust the exception list in the failing test.

By "exception list" I think you mean attributeOrderGrandfathered. I'll try that. Note that the earlier PR used attributeOrderGrandfathered (#5149).

-For currency, allow numberSystem after references

btangmu · 2025-12-02T17:17:26Z

I think you mean attributeOrderGrandfathered

That works. The tests are now passing.

Note that currency needs to be after validSubLocales to satisfy MergeLists ("cannot merge if we have [...A...B...] and [...B...A...]"), which has no exception list.

macchiati

Just one remaining issue

macchiati · 2025-12-02T22:18:27Z

tools/cldr-code/src/main/java/org/unicode/cldr/test/CheckNumbers.java

            }
        }

-        // Check that symbols are for an explicit number system;


Why is this being dropped? We still want to ensure that .../symbols has a numberSystem.

My mistake! I confused this section with the one below that's deleted. I'll fix that now.

Fixed by the 5th commit.

macchiati · 2025-12-02T22:20:34Z

Also, we will need documentation in the spec (doesn't have to be this PR, but the ticket can't be closed until that is done).

macchiati · 2025-12-03T21:02:06Z

tools/cldr-code/src/main/java/org/unicode/cldr/test/CheckNumbers.java

        }
        XPathParts parts = XPathParts.getFrozenInstance(path);

-        // TODO: re-enable this commented-out check


Hmmm. Rather than disable the following for all cases in order for currency to have it be optional, we probably want to just disable it for currency. Sorry, should have caught this earlier.

just disable it for currency

Done in the 6th commit

-Restore the long-ago disabled check but skip for NumericType.CURRENCY

CLDR-18963 Allow numberSystem attribute for currency element

4fb24ff

-Revise ldml.dtd to allow numberSystem attribute for currency element -Delete the (disabled) check for numberSystem in CheckNumbers.java

btangmu self-assigned this Nov 25, 2025

CLDR-18963 Allow numberSystem attribute for currency element

d67ac2c

-Move numberSystem after draft

btangmu mentioned this pull request Nov 25, 2025

CLDR-18963 Explicit numberSystem attribute #5149

Closed

1 task

CLDR-18963 Allow numberSystem attribute for currency element

5ad4713

-Move numberSystem after references and validSubLocales

btangmu marked this pull request as ready for review November 25, 2025 20:20

btangmu requested a review from macchiati November 25, 2025 20:20

srl295 previously approved these changes Nov 29, 2025

View reviewed changes

CLDR-18963 Fix attributeOrderGrandfathered

425710a

-For currency, allow numberSystem after references

btangmu dismissed srl295’s stale review via 425710a December 2, 2025 16:33

macchiati requested changes Dec 2, 2025

View reviewed changes

CLDR-18963 Correction, do not drop earlier part of CheckNumbers.java

8061f5e

btangmu requested a review from macchiati December 3, 2025 16:18

macchiati requested changes Dec 3, 2025

View reviewed changes

CLDR-18963 Re-enable numberSystem except for currency

34728cf

-Restore the long-ago disabled check but skip for NumericType.CURRENCY

CLDR-18963 Allow numberSystem attribute for currency element #5205

Are you sure you want to change the base?

CLDR-18963 Allow numberSystem attribute for currency element #5205

Conversation

btangmu commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

btangmu commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

macchiati commented Nov 26, 2025

Uh oh!

macchiati commented Nov 26, 2025

Uh oh!

btangmu commented Nov 26, 2025

Uh oh!

macchiati commented Nov 27, 2025 via email

Uh oh!

btangmu commented Nov 28, 2025

Uh oh!

macchiati commented Dec 1, 2025

Uh oh!

btangmu commented Dec 2, 2025

Uh oh!

btangmu commented Dec 2, 2025

Uh oh!

macchiati left a comment

Choose a reason for hiding this comment

Uh oh!

macchiati Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

btangmu Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

btangmu Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

macchiati commented Dec 2, 2025

Uh oh!

macchiati Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

btangmu Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

btangmu commented Nov 25, 2025 •

edited

Loading

btangmu commented Nov 25, 2025 •

edited

Loading