-
Notifications
You must be signed in to change notification settings - Fork 517
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
Fix panic in TimeZone::from_local_datetime
#1071
Fix panic in TimeZone::from_local_datetime
#1071
Conversation
7a3747c
to
fdf5bb0
Compare
It is a bit unfortunate that I think it is better to implement the conversion in Then we can guarantee that |
fdf5bb0
to
2c66f24
Compare
I also found a place in |
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.
LGTM
1564bfd
to
cb98162
Compare
cb98162
to
d1c6503
Compare
d1c6503
to
b6bebc6
Compare
b6bebc6
to
f3c0fa2
Compare
f3c0fa2
to
fa25a5c
Compare
86cd777
to
bc10f89
Compare
bc10f89
to
b5940e5
Compare
Codecov Report
@@ Coverage Diff @@
## 0.4.x #1071 +/- ##
==========================================
+ Coverage 91.21% 91.23% +0.01%
==========================================
Files 38 38
Lines 17121 17157 +36
==========================================
+ Hits 15617 15653 +36
Misses 1504 1504
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ee5a532
to
8a2cd95
Compare
8a2cd95
to
49ec1b3
Compare
This was based on a wrong assumption. So this is now back to a simple fix of the provided method |
88add13
to
9454970
Compare
This no longer depends on other PRs. |
src/offset/mod.rs
Outdated
@@ -527,10 +527,55 @@ pub trait TimeZone: Sized + Clone { | |||
} | |||
} | |||
|
|||
/// Helper function to map `LocalResult<FixedOffset>` to `LocalResult<DateTime<Local>>`. | |||
/// Returns `None` on out-of-range. | |||
fn local_time_min_offset<Tz: TimeZone>( |
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.
Since this has a single caller, let's just inline it?
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.
With a separate function I could use ?
, and convert to LocalResult::None
in the caller. It seemed the cleanest solution at the time.
Found a nicer way to write it now.
9454970
to
d7dfcf5
Compare
d7dfcf5
to
e50dd89
Compare
Thank you for all your reviews! |
Split out from #1048. Depends on #1069 for
NaiveDateTime::checked_sub_offset
, only the last commit is new.The change is that
TimeZone::from_local_datetime
should check for overflow instead of blindly subtracting the offset. But you can't do so easily inLocalResult::map
, so this adds a helper method.Note that this PR only fixes the default implementation. But the implementations of
TimeZone
forLocal
don't use the default implementation, and are not fixed yet.The last commit in #1017 (comment) refactors
Local
to pick up this fix.@djc do you want me to split out that commit and try to apply it to 0.4.x?Edit: Split it out to #1072.