Revise Months API #752
Revise Months API #752
Conversation
djc
force-pushed
the
checked-add-sub-months
branch
from
3c1e6bc
to
a1e3ca7
Compare
* Provide `checked_add_months()` and `checked_sub_months()` for callers that would like to avoid panics * Document panic potential in `Add` and `Sub` implementations * Implement additional traits for `Months` per API guidelines * Hide inner type for `Months` and add constructor * Use lower-level APIs to clamp day
djc
force-pushed
the
checked-add-sub-months
branch
from
a1e3ca7
to
46f2ebe
Compare
There was a problem hiding this comment.
This looks very clean and consistent with other chrono APIs with the checked_add_* functions. I've set up #753 with a potential change moving the adding months to a month logic to overflowing_add/sub functions in the month module, however feel free to ignore that if you think it doesn't make sense to have the overflowing_add/sub functions.
| return Some(self); | ||
| } | ||
|
|
||
| match months.0 <= core::i32::MAX as u32 { |
There was a problem hiding this comment.
Could we avoid this by having new_opt and new constructors on the Months which only accept valid u32 values?
There was a problem hiding this comment.
Note that core::i32::MAX and -core::i32::MIN are not the same value... in other words, the boundary here depends on the context in which it is used (in this case add vs sub). I'm inclined to not make this any more complex than it already is.
| // Clamp original day in case new month is shorter | ||
|
|
||
| let flags = YearFlags::from_year(year); | ||
| let feb_days = if flags.ndays() == 366 { 29 } else { 28 }; |
There was a problem hiding this comment.
This is a much better implementation, thank you! Had I known about ndays() I'd have done things differently. Either way, very happy to have support for adding Months in chrono!
| } | ||
|
|
||
| // Copy `i32::MIN` here so we don't have to do a complicated cast | ||
| match months.0 <= 2_147_483_648 { |
checked_add_months()andchecked_sub_months()forcallers that would like to avoid panics
AddandSubimplementationsMonthsper API guidelinesMonthsand add constructorSince I didn't get another chance to review #731 before it was merged (and the suggestion to make panicking explicit doesn't seem to have been followed up), here are some proposed improvements.
cc @avantgardnerio