Add ixdtf convenience functions to `icu_timezone`

unicode-org / icu4x

Solving i18n for client-side and resource-constrained environments.

https://icu4x.unicode.org

Other

1.32k stars 172 forks source link

Add ixdtf convenience functions to `icu_timezone` #5201

Open sffc opened 1 month ago

sffc commented 1 month ago

A convenience function we should definitely support:

impl From<UTCOffsetRecord> for GmtOffset

Probably there are a few more we should also add.

CC @nekevss

nekevss commented 1 month ago

I can take a look at this if no one has started it yet.

sffc commented 1 month ago

I wrote one function in https://github.com/unicode-org/icu4x/pull/5202. Unfortunately it can't be infallible because the UTCOffsetRecord fields are public, so they could represent an out-of-bounds offset.

sffc commented 1 month ago

Ok, in #5260 I added ixdtf parsing functions for Date, Time, and DateTime.

We should also add them to:

CustomTimeZone
CustomZonedDateTime

Similar to how the Date and DateTime AnyCalendar impls require #[feature = "compiled_data"], I think these ones should also have that requirement, and they should use compiled data to convert the IANA to BCP-47 time zone ID and look up the metazone. We need to leave the zone variant blank for now since there isn't a way to derive that from the IXDTF string without TZDB.

sffc commented 1 month ago

@nekevss I'm assigning this to you based on https://github.com/unicode-org/icu4x/issues/5201#issuecomment-2215575876.

sffc commented 6 days ago

I noticed that we have

CustomZonedDateTime::try_iso_from_ixdtf_str
DateTime::try_iso_from_str

Why does one have ixdtf in the name and the other not?

nekevss commented 6 days ago

Mainly, GmtOffset has try_from_str and from_str, which is also fairly visible in the docs. Meanwhile, the method for CustomTimeZone was going to be CustomTimeZone::try_from_str. Since the format of the input expected by the CustomTimeZone method (along with CustomZonedDateTime) are different than GmtOffset::try_from_str, the thought was to avoid confusion by being explicit in the method name CustomTimeZone::try_from_ixdtf_str. This naming then was extended to CustomZonedDateTime for consistency.

I do agree though that the method names being different between CustomZonedDateTime and DateTime are not ideal.

If you don't have too much concern about potential confusion between GmtOffset::try_from_str and CustomTimeZone::try_from_str, then I'd be fine with updating both to *_from_str or a half step of keeping CustomTimeZone::try_from_ixdtf_str and ZonedDateTime::try_iso_from_str

sffc commented 4 days ago

Discussion:

@sffc Can you write CustomTimeZone::try_from_ixdtf_str("America/Chicago") ?
@nekevss I don't think so yet.
@sffc Maybe what we want is:
- CustomTimeZone::try_from_iana_str (need to think about name) which takes strings like "America/Chicago" and "-0600" and does the best it can to populate the fields of CustomTimeZone
- CustomTimeZone::try_from_ixdtf_str that has current behavior
- CustomTimeZone::try_from_str that first tries iana and then ixdtf
@sffc As far as the other types, I think the IXDTF representation is the actual canonical representation in string form of CustomZonedDateTime and DateTime, so I think those functions should be called try_from_str
@sffc Actually maybe we just have CustomZonedDateTime::try_from_str and then you can just pull the time zone out of it.
@nekevss I like that.