unicode-org / icu4x

Solving i18n for client-side and resource-constrained environments.
https://icu4x.unicode.org
Other
1.29k stars 166 forks source link

Add Sedate IXDTF (extended ISO) string parsing function #2127

Open sffc opened 2 years ago

sffc commented 2 years ago

We should add a function that parses ISO-8601 strings. We should follow the Temporal grammar:

https://tc39.es/proposal-temporal/#sec-temporal-iso8601grammar

i.e., we should implement a string parser conforming to IETF Sedate that parses a string into its components (date, time, time zone, calendar), and use this parser in APIs and anywhere else we parse date strings. Think about where to put this code in the crate structure.

We should start with the DateTime production, and work our way up towards CalendarDateTime and TemporalZonedDateTimeString.

Examples of strings that are accepted as part of the DateTime grammar:

sffc commented 1 year ago

Note: currently we have https://unicode-org.github.io/icu4x-docs/doc/icu_datetime/mock/fn.parse_gregorian_from_str.html

We should start with a design doc for this. We likely want to end up with a parser type named something like IetfDateTimeStringParser that is able to support the whole grammar.

sffc commented 1 year ago

Question: do we want to have specific, stricter parsers for each individual type, as well as the fully-featured one conforming to IETF Sedate?

sffc commented 1 year ago

We need to design how the parser will be architected so that it can be easily implemented.

Discuss with:

Optional:

robertbastian commented 4 months ago

Do we want to leverage an existing implementation, such as the time crate?

nekevss commented 4 months ago

Hi! FWIW, we have an initial parsing implementation in temporal_rs. Although, it admittedly needs more testing and clean up.

Manishearth commented 4 months ago

Do we want to leverage an existing implementation, such as the time crate?

Open to doing so with a Cargo feature.

Though I think this is the kind of thing that is okay for us to expose if we wish.

sffc commented 4 months ago

I think a SEDATE parser should be its own crate. Maybe it lives in our repo, maybe in the temporal_rs repo, maybe but maybe not in the time or chrono repo.

sffc commented 4 months ago

I changed the title of this issue to emphasize that we're talking about IXDTF, which is not functionality I'm aware of in Rust except for temporal_rs mentioned above.

nekevss commented 4 months ago

Just to expand on my above comment. From what I can tell after reading through the IETF doc, I think the parser in temporal_rs can parse Sedate IXDTF or, at least, is fairly close to it. There might be a couple things missing, and the API definitely needs some work if the decision were to use that parser.

I'm pretty neutral on whether the functionality lives in icu4x, temporal_rs, or its own crate. I mostly figured that I would mention that it exists if there was interest in using the currently existing one as a base or just cleaning it up and using what's there. 😄

For reference, here are all the tests for that parser.

sffc commented 4 months ago

Thanks for the reference! Looking over that code, it seems to be well-written and suitable as a dependency of the icu4x project. I think it would be worthwhile pulling this code into an ixdtf crate (it looks like we are currently squatting that crate name). Seems harmless to include the Duration parsing code as well. The parser should return the raw IsoParseRecord type so that it isn't specific to the types in either icu_calendar or temporal_rs.

nekevss commented 4 months ago

Sounds good. I'll move the code over to the directory. update it with the above comments., and submit a PR

sffc commented 4 months ago

Discussion: after @nekevss's PR is landed, we will add ixdtf as an optional, default dependency of icu_calendar and use where necessary.

LGTM: @sffc @Manishearth @robertbastian

sffc commented 3 months ago

4646 basically fixes this issue but there were some minor follow-ups. I'm re-opening this issue to track that, or we can move those to another issue and re-close this one.

nekevss commented 3 months ago

I can definitely work on the follow ups for the ixdtf as needed. I think it makes sense to have a separate issue to track progress on any follow ups needed on ixdtf.

sffc commented 2 months ago

Schedule release review for the IXDTF crate

Start: 11:01

Options:

  1. Ship as 0.x when @sffc and @nekevss feel it is ready
  2. Ship as 0.x after an API review with the team
  3. Ship as 1.0 after an API review with the team

Discussion:

sffc commented 2 months ago

Checklist:

sffc commented 2 months ago

Plan:

1a. @sffc to finish the IXDTF checklist. 1b. @sffc to add ixdtf as a depedency of the icu_calendar crate and use it 1c. @nekevss to bring the docs up to par with the checklist requirements

  1. Release ixdtf at 0.x
  2. Add ixdtf as a depedency of the temporal_rs crate and use it
  3. We consider icu_calendar and temporal_rs experience to be sufficient for making a 1.0 release of ixdtf