Feat: Timezone improvements#71
Conversation
ptomato
force-pushed
the
proposed-clocks-improvements
branch
from
c8c3316 to
205bdc8
Compare
lann
left a comment
lann
left a comment
There was a problem hiding this comment.
I like this change overall. I've added some feedback on the actual changes plus some other notes that can be punted to a separate PR if you don't want to deal with them.
ptomato
force-pushed
the
proposed-clocks-improvements
branch
from
205bdc8 to
c91853b
Compare
|
I've rebased this on top of #79. The changes added in this PR can be seen in bakkot/wasi-clocks@clocks-improvements-on-three...ptomato:wasi-clocks:proposed-clocks-improvements. |
|
@ptomato, sorry, I rebased my PR and broke this one again. Working link to view changes in this PR: bakkot/wasi-clocks@f1a63db...ptomato:wasi-clocks:proposed-clocks-improvements |
ptomato
force-pushed
the
proposed-clocks-improvements
branch
from
c91853b to
0c3e042
Compare
|
OK, I pulled in Kevin's branches, this should be the comparison link now: ptomato/wasi-clocks@b340dbe...ptomato:wasi-clocks:proposed-clocks-improvements |
lann
left a comment
lann
left a comment
There was a problem hiding this comment.
One small suggestion but overall looking good. Thanks for this!
| iana-id: func() -> option<string>; | ||
|
|
||
| /// The same as `display`, but only return the UTC offset. | ||
| /// The number of nanoseconds difference between UTC time and the local |
There was a problem hiding this comment.
I'd be moderately inclined to leave this as seconds. There is some appeal to being consistent with duration but I think it makes more sense to match instant.seconds here.
From what I've been reading there shouldn't be any need for sub-second offsets.
There was a problem hiding this comment.
I'd encourage sticking with nanoseconds here — we found a number of cases in the tzdata where there were sub-second offsets. See Justin's original recommendation at #61 (comment)
If the concern is that it would be easier to just do instant.seconds - offset or whatever to get the local wall-clock time, then I'd suggest instead adding a method that does that, because you probably want wall-clock time to be expressed in Y-M-D-H-M-S and not epoch nanoseconds...
There was a problem hiding this comment.
https://tz.iana.narkive.com/mszSx6kS/fractional-seconds-in-zic-input
Who am I to argue with Paul Eggert about timezones? 😂
Though notably:
fractional-second times [... in tzdata ...] is not something we'd do lightly, if ever
There was a problem hiding this comment.
Yeah, I don't think it'd be a terrible mistake to round to seconds, but seems better to design the interface to accommodate the precision that's available from the data source, and let callers round according to their needs...
| /// not have an IANA identifier, this returns nothing. | ||
| @unstable(feature = clocks-timezone) | ||
| display: func(when: datetime) -> timezone-display; | ||
| iana-id: func() -> option<string>; |
There was a problem hiding this comment.
This is great. It will allow function like https://docs.rs/jiff/latest/jiff/tz/struct.TimeZone.html#method.try_system
to recognize None as an error if the system time is not available. Much better than before.
|
Up to you. I think @bakkot split them originally in order to discuss the renaming separately from the more structural changes in this PR. |
|
@bakkot Are we ready to move forward with these changes? |
|
Needs another rebase (sorry!) but the changes here seem great to me. |
The "timezone of an instant" is a misnomer because the `instant` type doesn't carry timezone information. The host has a currently configured time zone (or has no such concept, in which case the only sensible thing to do is use UTC).
This flag is problematic because when a time zone is "in DST" is not well defined. Most time zones in the world don't use DST at all. Of the ones that do, most go to DST during the summer for half the year, but not all. For example, the function in Moment.js that provides this functionality comes with a giant caveat: https://momentjs.com/docs/#/query/is-daylight-saving-time/
Other possibilities: "tzid", "iana-id", "identifier", "iana-identifier". Returning a user-displayable name as part of timezone-display would require more information: the user's preferred language, and the preferred style for the name: - abbreviated or not - year-round or specific to the Instant E.g., the time zone with the IANA id "America/Los_Angeles" might be displayed as "Pacific Time", "Pacific Standard Time", "Pacific Daylight Time", "PT", "PST", "PDT", "Nordamerikanische Westküstenzeit"... The Rust iana_time_zone crate uses IANA time zone IDs, so if this interface needs to be able to implement iana_time_zone, timezone-display should have an IANA ID and not a user-displayable name.
There are time zones that used sub-minute or even sub-second UTC offsets for instants in the past. E.g., when built using Vanguard format, the UTC offset in the TZDB for "Asia/Ho_Chi_Minh" before July 1906 is 7:06:30.133333333.
If the timezone-display ID is an IANA ID, and we are going with the
approach of not making the localized ("PST" vs "PDT" vs "PT") name part of
this component, then the current time zone doesn't depend on the current
time.
After removing the isDST flag, timezone-display contains two pieces of
data, the ID and the UTC offset. The UTC offset is already available via
a function that takes an Instant as input. The ID could just be available
via its own function that doesn't take any input.
In that case there would be no need for timezone-display.
Instead of only specifying the epoch of the instant returned from now(), specify the epoch of the instant type. It then follows that now() returns an instant with that epoch.
We wish to be able to represent "is completely unaware of time zones" or "failure to discover a time zone" in the API, distinct from a host that always uses UTC.
Use types::duration for the return type of get-resolution instead. It supports durations up to 584 years which is more than sufficient for any clock resolution. (A more capable duration type would be needed if we were doing any arithmetic between instants.)
ptomato
force-pushed
the
proposed-clocks-improvements
branch
from
e183e89 to
b9858f1
Compare
|
Rebased. If you are considering an instant-to-wall-clock-time method as I suggested in #71 (comment), I could draft that in another PR. I haven't done so here because that seems like it might need definition of a Y-M-D-H-M-S type, and that seems like a larger discussion. |
Yes. That sounds like it would be easy to add that in a new PR. |
cdmurph32
changed the title
4 participants
See WebAssembly/WASI#688 (comment). This is intended as a starting point for discussion about what's possible.