Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a "DateDelta" datatype for differences in number of days #858

Closed
wants to merge 1 commit into from

Conversation

esheppa
Copy link
Collaborator

@esheppa esheppa commented Oct 31, 2022

change Days to take a u32 and addition of DaysDelta to NaiveDate, changes to Iterator Implementations

This has been separated out from #824

@esheppa
Copy link
Collaborator Author

esheppa commented Oct 31, 2022

This could potentially target 0.4.x but that would preclude some of the API changes like the new impl Sub return type

@esheppa
Copy link
Collaborator Author

esheppa commented Nov 2, 2022

Actually, upon re-reading this, I think the only thing I'd have to change is to leave Days::new taking a u64, then this would be ok in 0.4.x

@djc
Copy link
Member

djc commented Nov 2, 2022

Sorry, I've paged out my context on why we needed/wanted this as an addition.

(If we want to do it, doing it on 0.4.x first seems like a great idea. Would also be nice to rewrite the commit message to have a shorter first line introduction.)

@esheppa
Copy link
Collaborator Author

esheppa commented Nov 2, 2022

Fair call - the main idea here is to have a proper return type for things like my_date.days_since(my_other_date) - where we want to use the existing Days type, but the difference can either be forwards or backwards.

@esheppa esheppa changed the base branch from main to 0.4.x November 2, 2022 11:02
@esheppa esheppa force-pushed the add-date-delta branch 2 times, most recently from 1678d91 to f9c9f9a Compare November 2, 2022 11:04
@djc
Copy link
Member

djc commented Nov 7, 2022

Been thinking about this more. What if instead of all the *Delta types we only offer checked_sub() operations that just yield None if negative? I feel like the proliferation of Delta types makes the API unwieldy and they actually have really limited utility since the sub of something negative is not something that actually makes all that much sense in this domain.

@esheppa
Copy link
Collaborator Author

esheppa commented Nov 8, 2022

This sounds reasonable to me. From the implementation, the only time *Delta is useful is when you have two datetimes/times and you want to get the difference in duration between them along with the ordering in one operation, but this is likely a rare use case.

What if we had the following functions:
checked_sub / checked_difference / checked_delta - this returns Option<core::time::Duration>
absolute_delta / abs_diff - this returns the absolute value in core::time::Duration

And then we would also not implement the ops::Sub<NaiveDate> for NaiveDate etc for NaiveDateTime as well. This is great as it means there would now be the full functionality of https://github.com/chronotope/chrono/pull/860/files available in 0.4.x!

@djc
Copy link
Member

djc commented Nov 8, 2022

Yeah, sounds good! I think I would go with checked_sub() and abs_diff().

* change Days to contain a u32
* changes to Iterator Implementations
* minor docs fixes
* add days_since function
@esheppa
Copy link
Collaborator Author

esheppa commented Nov 23, 2022

Had another thought of just using Result for this - what do you think? (with a similar pattern in #860)

@djc
Copy link
Member

djc commented Nov 24, 2022

What's the point of changing from u64 to u32?

@esheppa
Copy link
Collaborator Author

esheppa commented Nov 25, 2022

This was originally combined with the work in d086ca9 and in the implementations of signed_duration_since which is infallible, a u32 is much more convenient to work with than a u64, due to the From<u32> for i64 impl. In hindsight u64 was a mistake and u32 makes more sense for days, given that u32 allows for far more years than NaiveDate has in total range anyway. u16 would be too narrow, and so u32 is the most reasonable fit.

Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add attribute #[must_use] to new and modified functions.

@@ -2673,22 +2724,22 @@ mod tests {
assert_eq!(lhs.checked_add_days(rhs), sum);
}

check((2014, 1, 1), Days::new(0), Some((2014, 1, 1)));
check((2014, 1, 1), Days::from_u32(0), Some((2014, 1, 1)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backwards compatibility checks, could you add some tests cases (or statements within a test case) to compare Days::new and Days::from_u32 result in the same values?

I know new is the "old way", but wouldn't hurt to make sure it's still working.

@pitdicker pitdicker closed this Mar 17, 2024
@pitdicker pitdicker deleted the add-date-delta branch March 17, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants