Conversation
jedel1043
added
C-api
nekevss
left a comment
nekevss
left a comment
There was a problem hiding this comment.
Looks good to me!
one thought that's non-blocking. But either way, I'm fine with merging.
| /// `UtcOffsetSeconds` represents the amount of seconds we need to add to the UTC to reach the local time. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub struct TimeZoneOffset { | ||
| pub struct UtcOffsetSeconds(pub i64); |
There was a problem hiding this comment.
praise: yeah, this is a better name
| Ambiguous { | ||
| std: LocalTimeRecord, | ||
| dst: LocalTimeRecord, | ||
| std: UtcOffsetSeconds, |
There was a problem hiding this comment.
thought: maybe these should be changed from std/dst to standard/savings
General thought is to move away from the day light savings time reference. So savings is the offset that "saves" time so to speak. That may be incorrect though too because standard / savings might be a bit incorrect here (thinking mostly about dublin)
There was a problem hiding this comment.
Maybe standard and alternate then? It removes the "savings" terminology for something more general: an "alternate" timezone a region uses
There was a problem hiding this comment.
I think my only concern would be that it's not so much an alternate as it is a secondary. Weirdly enough the only good term I can think of is savings (but that's after working on all of the zoneinfo stuff).
I think icu_time uses VariantOffsets and they use standard and daylight.
There was a problem hiding this comment.
Hmm yeah, maybe this requires a bigger discussion on how to name timezone things. I'll merge this in the meantime and we can have a names discussion before releasing 0.1
2 participants
Makes it so that our timezone calculations don't depend on the
is_dstfield to calculate if a datetime is within a forward transition.I think this is a breaking change, but because the timezone providers are unstable, it should be fine.