Use NaiveDateTime for internal tz_info methods. #1658
Conversation
This replaces passing a timestamp and year to several internal functions with passing a NaiveDateTime, making the function signature slightly clearer. The values originated from in the a NaiveDateTime in first place, so it basically just postpones the conversion. Prep for chronotope#1628
src/offset/local/tz_info/rule.rs
Outdated
| ) -> Result<crate::MappedLocalTime<LocalTimeType>, Error> { | ||
| let current_year = local_time.year(); | ||
| let local_time = local_time.and_utc().timestamp(); | ||
|
|
||
| // Check if the current year is valid for the following computations |
There was a problem hiding this comment.
It should be impossible to fail the check below since (the current implementation of) NaiveDateTime has a max year of 262142. It could be removed, but perhaps we want to keep it in case NaiveDateTime ever gets changed?
There was a problem hiding this comment.
Have you checked what our test coverage is here? I think it's probably fine to remove the check here, maybe add a comment that we're relying on the invariant from the input NaiveDateTime?
There was a problem hiding this comment.
I've removed the check. I looks like this code isn't currently covered, but I'm already working on #1628 and tests there should cover this.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1658 +/- ##
=======================================
Coverage 91.02% 91.03%
=======================================
Files 37 37
Lines 17366 17361 -5
=======================================
- Hits 15808 15805 -3
+ Misses 1558 1556 -2 ☔ View full report in Codecov by Sentry. |
src/offset/local/tz_info/rule.rs
Outdated
| ) -> Result<crate::MappedLocalTime<LocalTimeType>, Error> { | ||
| let current_year = local_time.year(); | ||
| let local_time = local_time.and_utc().timestamp(); | ||
|
|
||
| // Check if the current year is valid for the following computations |
There was a problem hiding this comment.
Have you checked what our test coverage is here? I think it's probably fine to remove the check here, maybe add a comment that we're relying on the invariant from the input NaiveDateTime?
|
Thanks! |
When looking into doing #1628 I found this comment:
// should we pass NaiveDateTime all the way through to this fn?chrono/src/offset/local/tz_info/timezone.rs
Line 136 in 7cdca4b
It seemed a sensible bit of cleanup to do regardless of other changes, so here's a separate pull request for it.
This replaces passing a timestamp and year to several internal functions with passing a NaiveDateTime, making the function signature slightly clearer. The values originated from in the a NaiveDateTime in first place, so it basically just postpones the conversion.
Prep for #1628