-
Notifications
You must be signed in to change notification settings - Fork 594
Correctly order earliest / latest for Ambiguous times in local TimeZone #1626
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1626 +/- ##
==========================================
+ Coverage 91.12% 91.77% +0.65%
==========================================
Files 37 37
Lines 17137 17443 +306
==========================================
+ Hits 15616 16009 +393
+ Misses 1521 1434 -87 ☔ View full report in Codecov by Sentry. |
It's a matter of taste, be I find a list of assert_eq! statements to be clearer here.
djc
left a comment
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.
Thanks for working on this, the code changes look good and I appreciate the added test coverage.
| #[allow(clippy::bool_assert_comparison)] | ||
| fn test_dst_backward_tzfile() -> Result<(), Error> { | ||
| // Northern hemisphere DST (CET/CEST) | ||
| let data: [u8; 604] = [ |
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.
These big binary blobs are a pain to manage. What value does this test have over and above the test_dst_backward_posix_tz tests? Do these tests actually gain us any coverage?
|
Any news on this? |
|
We are working around this issue with this snippet of code: match /*...*/ {
MappedLocalTime::Ambiguous(mut earliest, mut latest) => {
// Bug in Chrono: https://github.com/chronotope/chrono/issues/1625
if earliest > latest {
std::mem::swap(&mut earliest, &mut latest);
}
// ...But it'd still be wise to make this a semver breaking change for anyone who has come to rely on the old behaviour. |
I asked a question which has not been answered. I'm not making any semver-compatible changes in the near future. |
This swaps earliest / latest in Local Timezone handling as it seems to be consistently in the incorrect order, see issue #1625 . I think I changed all places where an instance of MappedLocalTime::Ambiguous is created in the unix part of offset/local. I haven't yet looked at Windows.
I've also added some tests to cover this, which should cover most cases.