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

TimeZoneInfo should use DateTime #6235

Open
sffc opened this issue Mar 6, 2025 · 3 comments
Open

TimeZoneInfo should use DateTime #6235

sffc opened this issue Mar 6, 2025 · 3 comments
Labels
C-datetime Component: datetime, calendars, time zones S-tiny Size: Less than an hour (trivial fixes)

Comments

@sffc
Copy link
Member

sffc commented Mar 6, 2025

It still uses (Date<Iso>, Time).

@robertbastian

@sffc sffc added the C-datetime Component: datetime, calendars, time zones label Mar 6, 2025
@sffc sffc added this to the ICU4X 2.0 ⟨P1⟩ milestone Mar 6, 2025
@sffc sffc added the needs-approval One or more stakeholders need to approve proposal label Mar 6, 2025
@sffc
Copy link
Member Author

sffc commented Mar 6, 2025

Let's decide on #6238 first.

@robertbastian
Copy link
Member

I don't know, should it? .at_time could accept

  • (Date<Iso>, Time)
  • Date<Iso>, Time
  • DateTime<Iso>

I don't really see a point of using DateTime. We have the type for IXDTF parsing, and because in datetime we cannot use multiple arguments. If you look at the chrono_jiff example, we're separately building a Date and a Time, what's the point of wrapping it into a dumb DateTime just to pass it across the API?

@sffc
Copy link
Member Author

sffc commented Mar 6, 2025

#6235

  • @sffc It could be ZonedDateTime<Iso, Option<UtcOffset>>
  • @robertbastian I don't think we should have two UtcOffsets in TimeZoneInfo.
  • @sffc Yeah, ok. It makes sense to re-use the existing Option<UtcOffset> field. So then let's make the type DateTime<Iso>?
  • @robertbastian DateTime and ZonedDateTime are good output types for the parser and an input to DateTimeFormatter because they need to be one argument. But here, it can be two separate arguments. I don't want to overuse them
  • @sffc I prefer thinking of this model as, anywhere we need a tuple of date and time, we use the DateTime named tuple. We don't selectively downgrade to plain tuple or separate parameters
  • @robertbastian In FFI we always do, that would be more consistent
  • @sffc Yeah, these are Rust types
  • @robertbastian If you follow a rust_link from FFI you'll see a Rust signature that does not match the FFI signature, and a type that you cannot construct in FFI
  • @sffc The TimeZoneInfo::LocalTime is actually a datetime. It isn't conceptually a date/time tuple; it is a PlainDateTime.
  • @robertbastian - That statement doesn't make any sense, we don't distinguish between date+time and datetime, and I don't see a conceptual difference either
  • @sffc It's really a timestamp. It's a local timestamp because we don't have an offset all the time.
  • @robertbastian - and that can be perfectly well represented by a date and a time

No conclusion at this time.

@sffc sffc added S-tiny Size: Less than an hour (trivial fixes) and removed needs-approval One or more stakeholders need to approve proposal labels Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-datetime Component: datetime, calendars, time zones S-tiny Size: Less than an hour (trivial fixes)
Projects
None yet
Development

No branches or pull requests

2 participants