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 a ZonedTime type #5937

Open
sffc opened this issue Dec 20, 2024 · 4 comments
Open

Add a ZonedTime type #5937

sffc opened this issue Dec 20, 2024 · 4 comments
Assignees
Labels
C-datetime Component: datetime, calendars, time zones C-time-zone Component: Time Zones discuss Discuss at a future ICU4X-SC meeting discuss-priority Discuss at the next ICU4X meeting S-small Size: One afternoon (small bug fix or enhancement)

Comments

@sffc
Copy link
Member

sffc commented Dec 20, 2024

It can live next to ZonedDateTime.

Why: in order to use TimeFormatter to format a time and a zone together.

Other solutions considered but infeasible:

  1. Use ZonedDateTime: no, it has a calendar, but TimeFormatter has the () calendar
  2. Implement InFixedCalendar<()> for ZonedDateTime: no, this breaks type inference for everyone else
  3. Add format_any_calendar to TimeFormatter: possible, but seems confusing; ZonedTime is better

@Manishearth @robertbastian

@sffc sffc added C-datetime Component: datetime, calendars, time zones C-time-zone Component: Time Zones labels Dec 20, 2024
@sffc sffc added this to the ICU4X 2.0 Stretch ⟨P2⟩ milestone Dec 20, 2024
@Manishearth
Copy link
Member

no, this breaks type inference for everyone else

Important to note, this breaks type inference in an intractable way. A property we currently have is that let dtf = FixedCalendarDateTimeFormatter::new(YMD); dtf.format(datetime) automatically infers the correct calendar. There's no way to make FCDTF<(), ..> not care about calendars without having the above code also be valid for the no-calendar time case.

In general I consider passing e.g. a DateTime to a TimeFormatter to be a bit of an antipattern, it's nice that we support it and there are likely ways it will be convenient, but I don't want to encourage that. ZonedTime seems like a good way to avoid such constructs.

@Manishearth Manishearth added the S-small Size: One afternoon (small bug fix or enhancement) label Jan 7, 2025
@robertbastian
Copy link
Member

Why: in order to use TimeFormatter to format a time and a zone together.

Not to throw a wrench in the works, but I'm not convinced of this use case. A non-local time should always be accompanied by a date, because often it is on a different day. I believe it makes sense to require a date whenever a time zone is involved.

@robertbastian robertbastian added discuss Discuss at a future ICU4X-SC meeting discuss-priority Discuss at the next ICU4X meeting labels Jan 8, 2025
@sffc
Copy link
Member Author

sffc commented Jan 8, 2025

I mostly agree with you @robertbastian, but this opinion does not seem to be widely held, and people want to represent and display things like "10:00 PST".

For example, the ICU4X WG meeting is typically at 10:10 uslax on Thursdays. It is a zoned time, not specific to a date.

@Manishearth
Copy link
Member

A non-local time should always be accompanied by a date, because often it is on a different day

Things can be "daily at 10 Pacific". This is definitely something I've seen formatted relatively often in online spaces.

I think this is still a valid use case.

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 C-time-zone Component: Time Zones discuss Discuss at a future ICU4X-SC meeting discuss-priority Discuss at the next ICU4X meeting S-small Size: One afternoon (small bug fix or enhancement)
Projects
None yet
Development

No branches or pull requests

3 participants