-
Notifications
You must be signed in to change notification settings - Fork 533
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
Correctly order earliest / latest for Ambiguous times in local TimeZone #1626
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.
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?
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.