Closed korrat closed 2 years ago
Well it does not behave like assume_offset. It assumes the PrimitiveDateTime is in UTC. That system has not been designed to work on non-UTC date times. You should first convert all date times to UTC.
EDIT: Regarding documentation maybe I should make it clear but it's indeed UTC based. The PrimitiveDateTime you give to assume_timezone must be UTC otherwise the function behavior is unexpected. I thought the name of get_offset_utc
in the Timezone
trait made that clear. Maybe the function name assume_timezone
is also not clear.
Regarding the actual issue of assuming the PrimitiveDateTime is already in the proper offset, this would have to be a new API.
EDIT2: For completeness I think we should both methods but I have no idea about the API names. I will open issues towards that goal.
@korrat I've just pushed a new version 0.7.0 in the fix-assume-timezone branch (not yet in crates.io). That version includes a breaking change from 0.6 which uses the default offset for the given timezone in assume_timezone. This works with your test case and should work on any PrimitiveDateTime which is proper for the current timezone.
Does this new version 0.7 in master behave like you'd expect?
Regarding chrono-tz, unfortunately I can't achieve that much details with a PrimitiveDateTime
because a PrimitiveDateTime
is not convertible to a unix timestamp, so it's not possible to perform binary search directly on the PrimitiveDateTime
. To provide ambiguity checking, a new API function would have to be provided but on OffsetDateTime
which can be converted to a unix timestamp.
Thanks, @Yuri6037. While version 0.7 indeed behaves as I'd expect for that one test case, it fails two other test cases now:
2022-03-27T02:30+01:00
.2022-03-27T03:30+01:00
, but should be 2022-03-27T03:30+02:00
For naming, you might want to change the name of the method to something like assume_timezone_local
, to prevent silent changes for people relying on the UTC offset behaviour now.
As for chrono-tz, it seems that NaiveDateTime::timestamp
simply assumes UTC offset (https://docs.rs/chrono/latest/chrono/naive/struct.NaiveDateTime.html#method.timestamp) and chrono-tz adjusts the effective timestamps through some adjustment in FixedTimespanSet::local_span
. I don't understand why that works yet.
Thanks, @Yuri6037. While version 0.7 indeed behaves as I'd expect for that one test case, it fails two other test cases now:
- for a local date time of 02:30, which is invalid in CET, the result is
2022-03-27T02:30+01:00
.- for a local date time of 03:30, which is after DST changeover, the result is
2022-03-27T03:30+01:00
, but should be2022-03-27T03:30+02:00
For naming, you might want to change the name of the method to something like
assume_timezone_local
, to prevent silent changes for people relying on the UTC offset behaviour now.As for chrono-tz, it seems that
NaiveDateTime::timestamp
simply assumes UTC offset (https://docs.rs/chrono/latest/chrono/naive/struct.NaiveDateTime.html#method.timestamp) and chrono-tz adjusts the effective timestamps through some adjustment inFixedTimespanSet::local_span
. I don't understand why that works yet.
Thank you, I'll see what I can do. About the naming, it won't be a problem since this will be published under a new 1.0 release which should be completely independent from 0.6 (which is already issuing deprecation warning).
@korrat I've now included all necessary changes and also, for the sake of completeness, I've added ambiguity checking.
All test cases you gave me are now passing.
EDIT: Please check the new result in the crates.io pre-release version 1.0.0-alpha-1. Docs have also been updated. Please review the changes in the associated PR #8
Can confirm, all cases I can think of are handled nicely. Thanks a lot for being so quick to fix this!
When using
assume_timezone
on aPrimitiveDateTime
close to the forward DST changeover, I get an unexpected result for the offset in the end. In my opinion,time-tz
does not yet handle the timezone correctly in this situation.The following test demonstrates the issue. I've also included a test for the backward changeover, but I think the behaviour there is acceptable.
On 2022-03-27, the date of DST changeover in CET, 01:30 in CET is 00:30 in UTC, but 01:30 in UTC is 03:30 in CET due to DST changeover.
My expectation of
assume_timezone
was that it behaves similar toassume_offset
onPrimitiveDateTime
, which considers thePrimitiveDateTime
to be in the provided offset.The exhibited behaviour is correct when treating the
PrimitiveDateTime
as UTC, but I'd argue that this would not be very useful. If this is indeed the expected behaviour, the documentation should highlight this fact.I think the issue comes down to this line in
assume_timezone
:For completeness, I've included the behaviour of
chrono-tz
. Note that the second test panics, since the local time 02:30 is ambiguous during backward DST changeover in CET.