TimeZoneInfo
#5691
TimeZoneInfo
#5691
Conversation
robertbastian
requested review from
Manishearth,
zbraniecki and
a team
as code owners
| type TimeZoneIdInput = datetime_marker_helper!(@input/timezone/id, yes); | ||
| type TimeZoneMetazoneInput = datetime_marker_helper!(@input/timezone/metazone, yes); | ||
| type TimeZoneOffsetInput = datetime_marker_helper!(@input/timezone/offset, yes); | ||
| type TimeZoneVariantInput = datetime_marker_helper!(@input/timezone/variant, yes); | ||
| type TimeZoneLocalTimeInput = datetime_marker_helper!(@input/timezone/local_time, yes); |
There was a problem hiding this comment.
Thought: we could start being more strict with the input fields. Currently this code requires that the input type returns all four of the time zone input fields, even if using a format that doesn't require them. (Can be a low-priority follow-up)
| @@ -3025,6 +3087,8 @@ impl DateTimeNamesMarker for NeoSkeleton { | |||
| type ZoneGenericShort = datetime_marker_helper!(@names/zone/generic_short, yes); | |||
| type ZoneSpecificLong = datetime_marker_helper!(@names/zone/specific_long, yes); | |||
| type ZoneSpecificShort = datetime_marker_helper!(@names/zone/specific_short, yes); | |||
| type MetazoneLookup = datetime_marker_helper!(@names/zone/metazone_periods, yes); | |||
| type Offsets = datetime_marker_helper!(@names/zone/offsets, yes); | |||
There was a problem hiding this comment.
You mentioned today that you were thinking about making ZoneVariant required on the input.
I'm ultimately fine with this, but I do think that the zone variant calculator is useful to our clients, both now and in the future. So, if we do this, I think we should keep the zone variant calculator where it is, external to icu_datetime.
There was a problem hiding this comment.
I do think that the zone variant calculator is useful to our clients
I don't think it is. It performs a very small subset of TZ calculations, I don't really see how that is something we should support, and the rest of the calculations isn't. I'm happy making zone variant required, and not exposing a zone calculator. We also don't expose an offset calculator, which is very similar (goes from ID + datetime to offset).
There was a problem hiding this comment.
We also don't expose an offset calculator, which is very similar (goes from ID + datetime to offset).
These are two very different operations. Figuring out the zone variant requires a very similar shape of data as figuring out the metazone. Figuring out the actual offset requires full TZDB. Figuring out the zone variant involves looking at big ranges that last decades; figuring out the offset requires looking at the transitions each and every year.
My position is that ICU4X should support formatting of a standard full IXDTF string, which is time zone ID and offset.
You seem to think that the code you spent a few weeks writing for zone variant calculations is flawed, but I don't see how it is fundamentally flawed.
There was a problem hiding this comment.
My position is that ICU4X should support formatting of a standard full IXDTF string, which is time zone ID and offset.
I don't think IXDTF is the magical input format you want it to be.
| //! let time_zone = TimeZoneInfo { | ||
| //! time_zone_id: mapper.as_borrowed().iana_to_bcp47("America/Chicago"), | ||
| //! offset: Some(UtcOffset::from_eighths_of_hour(-6 * 8)), | ||
| //! local_time: Some((datetime.date, datetime.time)), |
There was a problem hiding this comment.
Thought: internally we always convert this to a local timestamp. Maybe we should take a LocalTimestamp (new type) instead of the weird tuple.
There was a problem hiding this comment.
A timestamp in local-time minutes is so odd, I don't want to expose that. This should stay a internal encoding of (Date<Iso>, Time).
There was a problem hiding this comment.
If we exposed it, I would say to use an i64 local-seconds timestamp, which is slightly less weird.
There was a problem hiding this comment.
The local thing is the big weirdness, not the minutes.
Co-authored-by: Shane F. Carr <[email protected]>
#5533