Implement generic assign operations for various types #925
Conversation
Includes: * `oldtime::Duration` * `datetime::DateTime` * `naive::date::NaiveDate` * `naive::datetime::NaiveDateTime` * `naive::time::NaiveTime` Did not implement it for `date::Date`. This is because `Add` expects to be passed the owned object, but `AddAssign` only has access to a mutable reference. Since `Date` doesn't implement `Copy`, that makes it impossible to generically and safely implement `AddAssign` using `Add` without cloning. It might be possible with unsafe code, but that's probably over the top. `date::Date` is deprecated anyways.
There was a problem hiding this comment.
Sorry to add some late comments to this PR.
These implementations make chrono a little nicer to use. Methods such as check_add_* have their use, but these AddAssign implementations are just convenient. So 👍 from me.
But: a generic implementation will not only affect chrono, but also library users.
If a user has impl Add<MyType> for NaiveDate and impl AddAssign<MyType> for NaiveDate, that would no longer compile because of the new generic implementation of AddAssign here, right?
This would be a (minor) breaking change for 0.4.x.
I don't think a blanked implementation is a good thing to have in general. Or said differently: if it is just a way to save some code in chrono, it is better to write the individual implementations.
| @@ -982,6 +962,30 @@ impl<Tz: TimeZone> Sub<Days> for DateTime<Tz> { | |||
| } | |||
| } | |||
|
|
|||
| impl<T, Tz: TimeZone> AddAssign<T> for DateTime<Tz> | |||
| where | |||
| NaiveDateTime: Add<T, Output = NaiveDateTime>, | |||
There was a problem hiding this comment.
This is not correct. The Add implementation of Days and Months with DateTime is timezone-aware. I think the correct way to do this is to just add the individual implementations.
|
|
||
| impl<T, Tz: TimeZone> SubAssign<T> for DateTime<Tz> | ||
| where | ||
| NaiveDateTime: Sub<T, Output = NaiveDateTime>, |
| @@ -381,6 +381,42 @@ impl Div<i32> for Duration { | |||
| } | |||
| } | |||
|
|
|||
| impl<T> AddAssign<T> for Duration | |||
There was a problem hiding this comment.
Probably best to stay clear of `Duration for now, see #1054.
|
@yottalogical would you be willing to add the individual assign operations? And @djc would you agree they can be useful? |
|
Closing due to inactivity. The related issue to track is #70. |
Includes:
oldtime::Durationdatetime::DateTimenaive::date::NaiveDatenaive::datetime::NaiveDateTimenaive::time::NaiveTimeDid not implement it for
date::Date. This is becauseAddexpects to be passed the owned object, butAddAssignonly has access to a mutable reference. SinceDatedoesn't implementCopy, that makes it impossible to generically and safely implementAddAssignusingAddwithout cloning. It might be possible with unsafe code, but that's probably over the top.date::Dateis deprecated anyways.