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

Add ixdtf convenience functions to icu_timezone #5201

Closed
sffc opened this issue Jul 8, 2024 · 7 comments
Closed

Add ixdtf convenience functions to icu_timezone #5201

sffc opened this issue Jul 8, 2024 · 7 comments
Assignees
Labels
C-time-zone Component: Time Zones

Comments

@sffc
Copy link
Member

sffc commented Jul 8, 2024

A convenience function we should definitely support:

  • impl From<UTCOffsetRecord> for GmtOffset

Probably there are a few more we should also add.

CC @nekevss

@sffc sffc added the C-time-zone Component: Time Zones label Jul 8, 2024
@nekevss
Copy link
Contributor

nekevss commented Jul 8, 2024

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

@sffc
Copy link
Member Author

sffc commented Jul 9, 2024

I wrote one function in #5202. Unfortunately it can't be infallible because the UTCOffsetRecord fields are public, so they could represent an out-of-bounds offset.

@sffc
Copy link
Member Author

sffc commented Jul 25, 2024

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
Copy link
Member Author

sffc commented Jul 25, 2024

@nekevss I'm assigning this to you based on #5201 (comment).

@sffc
Copy link
Member Author

sffc commented Aug 20, 2024

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
Copy link
Contributor

nekevss commented Aug 20, 2024

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
Copy link
Member Author

sffc commented Aug 22, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-time-zone Component: Time Zones
Projects
None yet
Development

No branches or pull requests

2 participants