-
Notifications
You must be signed in to change notification settings - Fork 19
Stop depending on is_dst
for calculations
#356
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
one thought that's non-blocking. But either way, I'm fine with merging.
#[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
pub struct TimeZoneOffset { | ||
pub struct UtcOffsetSeconds(pub i64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: yeah, this is a better name
// Note(nekevss): it may be best to switch this to initial, need to double check | ||
// disambiguation ops with inverse DST-STD relationship | ||
Ambiguous { | ||
std: LocalTimeRecord, | ||
dst: LocalTimeRecord, | ||
std: UtcOffsetSeconds, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Makes it so that our timezone calculations don't depend on the
is_dst
field 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.