Fix bug in temporal utilities due to DST being ignored. #955
Conversation
Codecov Report
@@ Coverage Diff @@
## master #955 +/- ##
==========================================
+ Coverage 82.31% 82.32% +0.01%
==========================================
Files 168 168
Lines 48715 48713 -2
==========================================
+ Hits 40100 40105 +5
+ Misses 8615 8608 -7
Continue to review full report at Codecov.
|
| None => $builder.append_null()?, | ||
| match $array.value_as_datetime(i) { | ||
| Some(utc) => { | ||
| let fixed_offset = match parse( |
There was a problem hiding this comment.
As written, I think this code will try and parse $tz for each row in the array (even though the $tz is a constant).
I wonder if it would be possible to try and parse $tz once to figure out if it was fixed (e.g. a Some<FixedOffset>, and then only check if that was None for each row before computing using_chrono_tz_and_utc_naive_date_time
There was a problem hiding this comment.
Thanks for picking this up @alamb - I have addressed this in a subsequent commit.
|
Thank you @novemberkilo -- I appreciate the follow up |
* Check behaviour of temporal utilities for DST. * Fix temporal util bug ignoring dst. * Refactor macro for efficiency.
* Check behaviour of temporal utilities for DST. * Fix temporal util bug ignoring dst. * Refactor macro for efficiency.
Which issue does this PR close?
Closes #901
What changes are included in this PR?
Replaces the function
using_chrono_tzwith a functionusing_chrono_tz_and_utc_naive_datetimeThe rationale for this is that an offset for a timezone depends on the point in time at which the offset is required -- it will vary depending on whether DST is in effect at that point in time or not.
Are there any user-facing changes?
I'm not sure.
using_chrono_tzis a publicly available function (albeit under thechrono-tzfeature flag) so strictly speaking the API is changing? If this is a concern, we could have both functions appear (withusing_chrono_tzremaining unused).What do you think @alamb ?