ICU-22668 Added unit tests to verify that CLDR-17892 addresses the or… #3184

richgillam · 2024-09-19T00:35:23Z

…iginal problem reported here.

Checklist

Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22668
Required: The PR title must be prefixed with a JIRA Issue number.
Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
Required: Each commit message must be prefixed with a JIRA Issue number.
Issue accepted (done by Technical Committee after discussion)
Tests included, if applicable
API docs and/or User Guide docs changed or added, if applicable

richgillam · 2024-09-19T00:36:36Z

icu4c/source/test/cintltst/udatpg_test.c

+    } TestCase;
+
+    const TestCase testCases[] = {
+        { "en_GB@calendar=iso8601;rg=uszzzz", u"EEEEyMMMMdjmm", u"y MMMM d, EEEE 'at' h:mm a" },


@macchiati Is it right that j gets us 12-hour time here? It seems like with ISO 8601, the default hour cycle should always be 24-hour.

This is ISO-based, but not strictly ISO 8601 (which is a constant ASCII format). So decided that what most users want as the alternative is the ordering, but still allowing for certain localized features (names of months, separators, etc).

richgillam · 2024-09-19T00:37:13Z

icu4c/source/test/cintltst/udatpg_test.c

+        { "en_GB@calendar=iso8601;rg=uszzzz", u"EEEEyMMMMdjmm", u"y MMMM d, EEEE 'at' h:mm a" },
+        { "en_GB@calendar=iso8601;rg=uszzzz", u"EEEEyMMMMdHmm", u"y MMMM d, EEEE 'at' HH:mm" },


@macchiati It doesn't seem like we should be getting the "at" here. I think the %atTime patterns from CLDR didn't make it over to ICU properly.

I'd still like some guidance on this issue. I sent a private email. This might actually be another instance of the problem described in https://unicode-org.atlassian.net/issues/ICU-22688 ...

Mark and I discussed this offline; I think this is a bug in the CLDR-to-ICU tooling. I'm going to file a new ticket.

richgillam · 2024-09-19T00:37:56Z

icu4c/source/test/cintltst/udatpg_test.c

+        { "en_US",                            u"Edjmm",         u"d EEE, h:mm a" },
+        { "en_US",                            u"EdHmm",         u"d EEE, HH:mm" },


@macchiati Regular US English wants d EEE? "12 Tuesday"? I don't think I've ever heard it expressed that way...

We decided on ordering y m d dow h m s z

We expect to be refining the patterns in later iterations. So this is just capturing what CLDR does now.

BTW ISO 8601 doesn't allow for the dow feature

This one isn't ISO 8601, though. It's just generic US English (for comparison). So is your comment still operative?

richgillam · 2024-09-19T00:39:34Z

icu4j/main/core/src/test/java/com/ibm/icu/dev/test/format/DateTimeGeneratorTest.java

+            { "en_US",                            "EEEEyMMMMdHmm", "EEEE, MMMM d, y 'at' HH:mm" },
+            { "en_US",                            "Edjmm",         "d EEE, h:mm a" },
+            { "en_US",                            "EdHmm",         "d EEE, HH:mm" },
+        };


This test is actually failing for me on the Java side. Was there some additional step I (or somebody else) needed to take to get the ISO 8601 changes into the ICU4J data JAR file?

Hmmm. I think the goal of this test should be to reflect what is in CLDR right now, and refine later. What are the symptoms of the failures now?

Well, I copied the same test from C++ to Java and it passes on C++ and fails on Java, which makes me think my copy of ICU4J didn't pick up the CLDR changes. That probably means I skipped a step somewhere, but I don't know what I forgot to do. Or, if this should have happened automatically with the CLDR integration, maybe something was missed?

Oops. It was my own fault. There was a hack in the actual code that I had forgotten I needed to take out. Sorry for the extra churn.

roubert · 2024-09-19T12:49:00Z

icu4c/source/test/cintltst/udatpg_test.c

+        if (assertSuccess("Error creating dtpg", &err)) {
+            UChar actualPattern[200];
+
+            udatpg_getBestPatternWithOptions(dtpg, testCases[i].skeleton, -1, 0, actualPattern, 200, &err);


See my review comments on the previous PR #3103: Please don't hardcode lengths, that becomes increasingly brittle when code changes over time, use UPRV_LENGTHOF(actualPattern) instead of 200.

I agree (though if Peter has limited time to fix this PR, it could be fixed later.)

I think you're both being overly picky, but I made the change.

You already made those changes in PR #3103 so even if you don't think it's particularly important I just don't understand why you'd prefer to throw those improvements away instead of simply copy-pasting that improved code into this PR.

I think I changed it over there because somebody (probably you) complained there. I didn't take it out intentionally; I started over from scratch because I didn't remember I had that other PR still open. Regardless, the fix is back in now...

I see you went ahead and made this change on your own. I thought I had made it in my last commit; but maybe I forgot that file.

richgillam · 2024-09-20T20:13:47Z

Can I please get somebody to approve this? Are there changes somebody is still waiting for me to make?

…iginal problem reported here and took out an earlier hack in the Java code to work around the original problem.

jira-pull-request-webhook · 2024-09-20T22:24:23Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

richgillam requested review from roubert, macchiati and pedberg-icu September 19, 2024 00:35

richgillam commented Sep 19, 2024

View reviewed changes

roubert reviewed Sep 19, 2024

View reviewed changes

markusicu assigned macchiati Sep 19, 2024

macchiati approved these changes Sep 20, 2024

View reviewed changes

ICU-22668 Added unit tests to verify that CLDR-17892 addresses the or…

0c92e87

…iginal problem reported here and took out an earlier hack in the Java code to work around the original problem.

richgillam force-pushed the ICU-22668-iso-8601a branch from 89060a0 to 0c92e87 Compare September 20, 2024 22:24

richgillam merged commit 9adcf8b into unicode-org:main Sep 20, 2024
100 checks passed

richgillam deleted the ICU-22668-iso-8601a branch September 20, 2024 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICU-22668 Added unit tests to verify that CLDR-17892 addresses the or… #3184

ICU-22668 Added unit tests to verify that CLDR-17892 addresses the or… #3184

richgillam commented Sep 19, 2024 •

edited

Loading

richgillam Sep 19, 2024

macchiati Sep 19, 2024

richgillam Sep 19, 2024

richgillam Sep 19, 2024

richgillam Sep 20, 2024

richgillam Sep 19, 2024

macchiati Sep 19, 2024

macchiati Sep 19, 2024

richgillam Sep 19, 2024

richgillam Sep 19, 2024

macchiati Sep 19, 2024

richgillam Sep 19, 2024

richgillam Sep 19, 2024

roubert Sep 19, 2024

macchiati Sep 19, 2024

richgillam Sep 19, 2024

roubert Sep 20, 2024

richgillam Sep 20, 2024

richgillam Sep 20, 2024

richgillam commented Sep 20, 2024

jira-pull-request-webhook bot commented Sep 20, 2024

		{ "en_GB@calendar=iso8601;rg=uszzzz", u"EEEEyMMMMdjmm", u"y MMMM d, EEEE 'at' h:mm a" },
		{ "en_GB@calendar=iso8601;rg=uszzzz", u"EEEEyMMMMdHmm", u"y MMMM d, EEEE 'at' HH:mm" },

		{ "en_US", u"Edjmm", u"d EEE, h:mm a" },
		{ "en_US", u"EdHmm", u"d EEE, HH:mm" },

ICU-22668 Added unit tests to verify that CLDR-17892 addresses the or… #3184

ICU-22668 Added unit tests to verify that CLDR-17892 addresses the or… #3184

Conversation

richgillam commented Sep 19, 2024 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richgillam commented Sep 20, 2024

jira-pull-request-webhook bot commented Sep 20, 2024

richgillam commented Sep 19, 2024 •

edited

Loading