CLDR-17938 Add test data generator for date time formatting #4011

echeran · 2024-09-03T16:27:54Z

This PR completes the ticket.

ALLOW_MANY_COMMITS=true

macchiati · 2024-09-03T17:32:08Z

tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateDateTimeTestData.java

+
+import java.io.IOException;
+import java.util.Arrays;
+import org.pcollections.HashPMap;


We try not to introduce too many dependencies on on other libraries. It seems like this could just be done with regular collections or guava collections.

I support that concern generally. The reason I thought to introduce a persistent collection library is that there are multiple dimensions that need to be iterated over (generate combinations for all calendar systems, for all selected dates, for all selected times, for all ways of specifying style/length (skeleton vs. style options), etc.). In particular, the fields for skeleton and the style options in a test case should be mutually exclusive -- if one has data, then the other should be empty.

Using immutable maps across the nested loops eliminates chances for unintended consequences of stateful mutation. I strongly prefer immutable maps for this reason, and thus do not want to use regular collections. But that also means I need to append to immutable collections.

I didn't immediately see it in the Guava APIs, but I guess it is possible, although doing so is clunky. Going this route would compel me to write helper methods to reduce that boilerplate, which would feel like I'm partially recreating the persistent collections library that point.

On the other hand, persistent collections are designed for supporting immutability + append/remove/etc., and have the added benefit of code readability b/c of concision when you do perform those operations. The PCollections library has no dependencies and 75 KB of jar size, FWIW.

I'm happy to do what you suggest, here. I would prefer that it entail immutable data structures -- either via Guava or PCollections.

We don't really have time to do an analysis of a new package to see if it introduces any new security / performance issue If you are not modifying these structures once built, it is pretty easy to just build them and convert to immutable structures.

Okay, removed PCollections as a dep, using the existing Guava dep's collections instead.

srl295 · 2024-09-03T18:14:55Z

PR title needs to start with CLDR-… as should the commit

macchiati · 2024-09-03T20:41:14Z

tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateDateTimeTestData.java

+
+import java.io.IOException;
+import java.util.Arrays;
+import org.pcollections.HashPMap;


We don't really have time to do an analysis of a new package to see if it introduces any new security / performance issue If you are not modifying these structures once built, it is pretty easy to just build them and convert to immutable structures.

srl295

i don't see the argument for the pcollections
as to iterating - that data should be pulled from the DTD anyway instead of hard coded.

srl295 · 2024-09-03T20:58:59Z

tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateDateTimeTestData.java

+          emptyMapStringToObject.plus("timeZoneName", "longGeneric")
+      ));
+
+  private static final PVector<String> CALENDARS =


duplicate of other supplemental data

srl295 · 2024-09-03T21:00:14Z

tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateDateTimeTestData.java

+  private static final PVector<String> TIME_ZONES =
+      TreePVector.from(Arrays.asList(
+          "America/Los_Angeles",
+          "Africa/Luanda",
+          "Asia/Tehran",
+          "Europe/Kiev",
+          "Australia/Brisbane",
+          "Pacific/Palau",
+          "America/Montevideo"
+      ));


other code uses ImmutableSet.of("…", "…", "…") - does this provide value beyond that?

I switched over to using Guava according to Mark's guidance, so it's a moot point now.

(There's a bit of cloning during runtime and verbosity of code that could be avoided if one were to use persistent data structures.)

srl295 · 2024-09-03T21:00:30Z

tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateDateTimeTestData.java

+
+  private static final HashPMap<String,Object> emptyMapStringToObject = HashTreePMap.empty();
+
+  private static final PVector<PMap<String, Object>> SPEC_OPTIONS =


Couldn't this just be read from the DTD?

echeran · 2024-09-04T00:26:27Z

@macchiati @srl295 Do you know what the proper way to format a date within CLDR code is? @sffc and I found a few separate instances of date time formatting, such as in cldr-code -> org.unicode.cldr.draft.Misc#getFormatted that uses ICU4J's DateTimePatternGenerator and SimpleDateFormat. Is that the way to do it?

macchiati · 2024-09-04T17:36:17Z

There is a class called ICUServiceBuilder, that patches ICU calls with CLDR data. (If you are doing something new we might need to add APIs to that.)

srl295 · 2024-09-04T17:55:48Z

a good source of example code would be the ExampleGenerator.java

echeran · 2024-09-09T23:27:23Z

Thanks @macchiati and @srl295 . @sffc and I took a look at ExampleGenerator.java and found in handleDateFormatItem that finds the right xpaths and the values at that those xpaths for the date/time patterns, and then calls icuServiceBuilder.getDateFormat(...) using those values in the call site.

There's a lot of important and necessary code in that method that would be better off refactored into a reusable place rather than taking a copy-and-paste shortcut. The method itself has a lot of copy and-pasted code 3x over for full/long/short date format patterns. We think it would be good if we also cleaned up & refactored the duplicated code as a part of creating the test data generator. WDYT?

macchiati · 2024-09-09T23:42:29Z

Sounds good; pulling useful methods out of ExampleGenerator (etc). There is a pending PR (from one of the interns) that refactors ExampleGenerator, so you should wait until that is merged to avoid conflicts.

…

On Mon, Sep 9, 2024 at 4:27 PM Elango Cheran ***@***.***> wrote: Thanks @macchiati <https://github.com/macchiati> and @srl295 <https://github.com/srl295> . @sffc <https://github.com/sffc> and I took a look at ExampleGenerator.java and found in handleDateFormatItem that finds the right xpaths and the values at that those xpaths for the date/time patterns, and then calls icuServiceBuilder.getDateFormat(...) using those values in the call site. There's a lot of important and necessary code in that method that would be better off refactored into a reusable place rather than taking a copy-and-paste shortcut. The method itself has a lot of copy and-pasted code 3x over for full/long/short date format patterns. We think it would be good if we also cleaned up & refactored the duplicated code as a part of creating the test data generator. WDYT? — Reply to this email directly, view it on GitHub <#4011 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMC5YRCZKA2SAXSKEFDZVYVHFAVCNFSM6AAAAABNSQEI36VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZZGMZDIMJZGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

srl295 · 2024-09-10T14:31:11Z

@echeran dont' forget to open a ticket and update the PR title and commit messages.

sffc · 2024-09-11T00:11:39Z

Sounds good; pulling useful methods out of ExampleGenerator (etc). There is a pending PR (from one of the interns) that refactors ExampleGenerator, so you should wait until that is merged to avoid conflicts.

Is it PR #3895?

It looks like it is just stuck on a linter. @echeran pushed the lint fix to the branch.

macchiati · 2024-09-12T17:16:39Z

Still failing spotless, build — and is draft. Elango, do you want to try to fix this for v46, or punt to 47?

srl295 · 2024-09-12T17:25:43Z

yes thanks for merging #3895

echeran · 2024-09-16T23:59:28Z

Still failing spotless, build — and is draft. Elango, do you want to try to fix this for v46, or punt to 47?

Still trying to get this done ASAP in case it can fit for v46. When is the last call for v46?

macchiati · 2024-09-17T00:14:51Z

Any test data that is targeted at ICU should be in before their freeze date (Thursday??).

…

On Mon, Sep 16, 2024 at 4:59 PM Elango Cheran ***@***.***> wrote: Still failing spotless, build — and is draft. Elango, do you want to try to fix this for v46, or punt to 47? Still trying to get this done ASAP in case it can fit for v46. When is the last call for v46? — Reply to this email directly, view it on GitHub <#4011 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMAESUBCANADM572AWLZW5WHNAVCNFSM6AAAAABNSQEI36VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJUGIZTINJXGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

echeran · 2024-09-18T22:47:25Z

Any test data that is targeted at ICU should be in before their freeze date (Thursday??).

Things are looking good and complete now. The only thing is the Jira ticket isn't accepted yet. The key files are GenerateDateTimeTestData.java and its output, datetime.json.

sffc · 2024-09-19T00:53:08Z

The data currently looks like

  {
    "dateStyle": "short",
    "timeStyle": "short",
    "calendar": "gregorian",
    "formatType": "standard",
    "locale": "en-US",
    "dateLength": "short",
    "timeLength": "short",
    "timeZone": "Pacific/Palau",
    "input_string": "1969-07-16T16:00+09:00[Pacific/Palau]",
    "verify": "7/16/69, 4:00 PM"
  },

A few suggestions for improvement:

Remove formatType since we aren't using it and it is a bad name (unclear what it means)
Keep dateStyle/timeStyle or dateLength/timeLength but not both
Remove timeZone since it is already in the input string
Rename the field "input_string" to "input" or "datetime"
Rename the field "verify" to "output" or "expected"

echeran · 2024-09-19T22:18:49Z

A few suggestions for improvement:

Done. All suggestions were good. PTAL.

macchiati · 2024-09-20T17:27:30Z

tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateDateTimeTestData.java

@@ -513,11 +513,11 @@ private static ImmutableMap<Object, Object> getTestCaseForZonedDateTime(
                        dateTimeGluePatternFormatType,
                        icuServiceBuilder);

-        // "input_string" = the ISO 18601 UTC time zone formatted string of the zoned date time
-        optionsBuilder.put("inputString", zdt.toString());
+        // "input" = the ISO 18601 UTC time zone formatted string of the zoned date time


ISO - International Organization for Standardization

ISO 18601:2013 - Packaging and the environment — General requirements for the use of ...

Suggested change

// "input" = the ISO 18601 UTC time zone formatted string of the zoned date time

// "input" = the ISO 8601 UTC time zone formatted string of the zoned date time

macchiati · 2024-09-20T17:28:26Z

I can't review the Jason file right now, because it's too big for the browser.

echeran · 2024-09-20T17:49:39Z

I can't review the Jason file right now, because it's too big for the browser.

For the purposes of just quick reviewing, you can view the PR files including the large JSON file by replacing the .com with .dev in the PR URL, and it spins up VS Code in the browser to make viewing the files manageable:

https://github.dev/unicode-org/cldr/pull/4011

(If you had to do it at the command line, you would have to check out the branch via gh pr checkout 4011 and navigate yourself to the file, which takes a few more steps.)

macchiati

Thanks for the tip, Elango. Data looks ok to me.

(I was on a tablet, and so constained in how I could review.)

(longer term, for each testdata file I think we want to have a unittest in CLDR; and in the testdata file point to the generator and the unit test. That will make it easier to manage.)

echeran · 2024-09-23T16:39:27Z

Blocked on the Jira ticket being accepted. Everything else is ready to go.

srl295 · 2024-09-26T16:37:47Z

tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateDateTimeTestData.java

+    private static final ImmutableSet<String> NUMBERING_SYSTEMS =
+            ImmutableSet.of("latn", "arab", "beng");
+
+    // Use underscores for locale id b/c of CLDR historical reasons, even though dash is preferable


could be in bcp47 and then convert them to CLDR using ULocale.forLanguageTag

srl295

OK to move forward (minor comments)

See unicode-org#4011

jira-pull-request-webhook · 2024-09-26T20:01:07Z

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

~ Your Friendly Jira-GitHub PR Checker Bot

github-actions bot assigned echeran Sep 3, 2024

macchiati reviewed Sep 3, 2024

View reviewed changes

macchiati requested changes Sep 3, 2024

View reviewed changes

srl295 reviewed Sep 3, 2024

View reviewed changes

echeran changed the title ~~Add test data generator for date time formatting~~ CLDR-17938 Add test data generator for date time formatting Sep 10, 2024

echeran marked this pull request as ready for review September 18, 2024 22:42

echeran requested review from macchiati and srl295 September 18, 2024 22:47

macchiati reviewed Sep 20, 2024

View reviewed changes

macchiati approved these changes Sep 21, 2024

View reviewed changes

srl295 reviewed Sep 26, 2024

View reviewed changes

srl295 approved these changes Sep 26, 2024

View reviewed changes

CLDR-17938 Add test data generator for date time formatting

0d14e89

See unicode-org#4011

echeran force-pushed the testdata-gen-datetime branch from f075050 to 0d14e89 Compare September 26, 2024 20:01

echeran merged commit ed98270 into unicode-org:main Sep 26, 2024
12 checks passed


		private static final HashPMap<String,Object> emptyMapStringToObject = HashTreePMap.empty();

		private static final PVector<PMap<String, Object>> SPEC_OPTIONS =

	// "input" = the ISO 18601 UTC time zone formatted string of the zoned date time
	// "input" = the ISO 8601 UTC time zone formatted string of the zoned date time

CLDR-17938 Add test data generator for date time formatting #4011

CLDR-17938 Add test data generator for date time formatting #4011

Conversation

echeran commented Sep 3, 2024 • edited Loading

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

srl295 commented Sep 3, 2024

Choose a reason for hiding this comment

srl295 left a 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

echeran commented Sep 4, 2024

macchiati commented Sep 4, 2024

srl295 commented Sep 4, 2024

echeran commented Sep 9, 2024

macchiati commented Sep 9, 2024 via email

srl295 commented Sep 10, 2024

sffc commented Sep 11, 2024

macchiati commented Sep 12, 2024

srl295 commented Sep 12, 2024

echeran commented Sep 16, 2024

macchiati commented Sep 17, 2024 via email

echeran commented Sep 18, 2024

sffc commented Sep 19, 2024

echeran commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macchiati commented Sep 20, 2024

echeran commented Sep 20, 2024

macchiati left a comment

Choose a reason for hiding this comment

echeran commented Sep 23, 2024

Choose a reason for hiding this comment

srl295 left a comment

Choose a reason for hiding this comment

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

echeran commented Sep 3, 2024 •

edited

Loading