chrono icon indicating copy to clipboard operation
chrono copied to clipboard

Use `overflowing_naive_local` in `DateTime::checked_add*`

Open pitdicker opened this issue 2 years ago • 1 comments
trafficstars

The final part of https://github.com/chronotope/chrono/pull/1048.

Using overflowing_naive_local in DateTime::checked_(add|sub)_days and DateTime::checked_(add|sub)_months prevents panics.

I thought it would be good if users can rely on another property: that a clear no-op works as a no-op. I.e.DateTime::checked_add_days(Days::new(0)) should always return the original value. To do that, checked_(add|sub)_days and checked_(add|sub)_months have a fast path that returns Some(self) if it is a no-op. They already had a fast path anyway, we just start relying on it.

with_year gains a fast path to have the same property.

What we can't promise is that you can add or subtract and create a value that is just out of range for the local time but still in range for the UTC value. That would require alternative methods on NaiveDate for add_days, checked_*_months and with_year that have a different bound checks. It just doesn't seem worth it to me.

pitdicker avatar Sep 28 '23 16:09 pitdicker

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.14%. Comparing base (02c68d6) to head (f23dcba).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1333      +/-   ##
==========================================
+ Coverage   92.11%   92.14%   +0.03%     
==========================================
  Files          40       40              
  Lines       18026    18089      +63     
==========================================
+ Hits        16604    16668      +64     
+ Misses       1422     1421       -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 28 '23 16:09 codecov[bot]

@djc May I ask for this PR as a next one to review? If @Zomtir wants to work on NaiveDate::checked_add* and NaiveDateTime::checked_add* (and with_year...) it would be nice to have these fixes in.

pitdicker avatar Feb 29 '24 07:02 pitdicker

Thank you!

pitdicker avatar Feb 29 '24 12:02 pitdicker