Skip to content
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

Fix date time format in ICU4J, ICU4C, ICU4X #297

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

sven-oly
Copy link
Collaborator

@sven-oly sven-oly commented Sep 4, 2024

This updates the timezone generator to output Instant data of the form in UTC:
"2024-03-07T00:00:01.00Z"

This instant is used to create the expected result with a given calendar and timezone.

The Java code parses this instant, then applies the options for timezone and calendar to create a formatted output.

Java: Also update to handle date style and time style options better.

This removes "und" from the locales generated in test data because that locale doesn't work the same was across different platforms.

closing #266 #267

@sffc
Copy link
Member

sffc commented Sep 5, 2024

"2024-03-07T00:00:01.00Z" is the instant that you want to format.

You want to format the instant in a particular time zone and calendar. You pass those values in the options bag. You should not need to put them into the IXDTF string. If you do, there is a conflict.

@sven-oly sven-oly changed the title Fix date time format in ICU4J Fix date time format in ICU4J & ICU4C Sep 12, 2024
@sven-oly sven-oly changed the title Fix date time format in ICU4J & ICU4C Fix date time format in ICU4J, ICU4C, ICU4X Sep 17, 2024
@sven-oly
Copy link
Collaborator Author

ICU4X has multiple fixes in the latest commit, fixing about 1/2 of all the test failures.

  1. Now using timezone offset
  2. Using date and time styles properly
  3. Using the skeleton string with a non-public API call

There are still test failures in formatting, including issues with using 12 vs. 24 hour time based on the locale. Also, there are details of formatting the GMT offsets, e.g., ICU4X gives "GMT +09:00" vs. "GMT+9" as expected.

@sven-oly
Copy link
Collaborator Author

PTAL

testgen/generators/datetime_gen.js Outdated Show resolved Hide resolved
testgen/generators/datetime_gen.js Outdated Show resolved Hide resolved
Comment on lines 430 to +432
let temporal_date = temporal_dates[date_index];
temporal_date['timeZone'] = timezone;
let zdt = Temporal.ZonedDateTime.from(temporal_date);
let temporal_instant = zdt.toInstant();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move these three lines to between 444 and 445, which is closer to where they're first used.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only comment that hasn't been addressed.

@sven-oly
Copy link
Collaborator Author

PTAL!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix time zone parsing in ICU4J DateTime executor Fix failing tests in CPP Date/Time format.
3 participants