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

Deprecate panicking constructors of TimeDelta #1450

Merged
merged 13 commits into from
Mar 6, 2024

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Feb 21, 2024

This PR touches a lot of lines, but I would only call the first 5 commits and the last one interesting.

  • In a few places that only work with dates I replaced TimeDelta::days and TimeDelta::weeks with the Days type, which should be a little faster and conceptually a better match.
  • In NaiveDateTime::{checked_add_signed, checked_sub_signed} we could remove a workaround now that we have try_seconds.
  • In Parsed::to_naive_date I factored out the logic to calculate a date from a week starting on Monday or Sunday to resolve_week_date. And instead of calculating a TimeDelta with the number of days to add to January 1 we just calculate the ordinal directly and set the ordinal.
  • All other commits replace each method that is to be deprecated with its try_ variant.

Fixes #1408.

Copy link

codecov bot commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.03%. Comparing base (0aa46dd) to head (563c53d).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1450      +/-   ##
==========================================
+ Coverage   91.95%   92.03%   +0.07%     
==========================================
  Files          40       40              
  Lines       18035    18163     +128     
==========================================
+ Hits        16584    16716     +132     
+ Misses       1451     1447       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pitdicker pitdicker force-pushed the time_delta_cleanup branch 4 times, most recently from 578771b to cf8765e Compare February 22, 2024 12:47
@pitdicker pitdicker marked this pull request as ready for review February 22, 2024 12:53
@pitdicker pitdicker changed the title Prepare TimeDelta for deprecating panicking constructors Deprecate panicking constructors of TimeDelta Feb 22, 2024
@@ -1140,7 +1140,8 @@ impl NaiveDate {
let (year2_div_400, year2_mod_400) = div_mod_floor(year2, 400);
let cycle1 = yo_to_cycle(year1_mod_400 as u32, self.ordinal()) as i64;
let cycle2 = yo_to_cycle(year2_mod_400 as u32, rhs.ordinal()) as i64;
TimeDelta::days((year1_div_400 as i64 - year2_div_400 as i64) * 146_097 + (cycle1 - cycle2))
let days = (year1_div_400 as i64 - year2_div_400 as i64) * 146_097 + (cycle1 - cycle2);
expect!(TimeDelta::try_days(days), "range of NaiveDate is MUCH less than TimeDelta")
Copy link
Member

Choose a reason for hiding this comment

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

This error message is a bit too opinionated for my taste, can we just say it is out of range?

Copy link
Collaborator Author

@pitdicker pitdicker Mar 6, 2024

Choose a reason for hiding this comment

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

It is const but otherwise functionally the same as

TimeDelta::try_days(days).unwrap() // range of NaiveDate is MUCH less than TimeDelta

The doc comment a little above says:

    /// This does not overflow or underflow at all,
    /// as all possible output fits in the range of `TimeDelta`.

The range of TimeDelta is ca. 585 million years, the range of NaiveDate ca. 525.000 years.

I can change it to "always in range".

Copy link
Member

Choose a reason for hiding this comment

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

That works for me.


let date = try_opt!(self.date.checked_add_signed(TimeDelta::seconds(rhs)));
let (time, rhs_days) = self.time.overflowing_add_signed(rhs);
let rhs_days = try_opt!(TimeDelta::try_seconds(rhs_days));
Copy link
Member

Choose a reason for hiding this comment

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

It seems pretty confusing to pass something called rhs_days to try_seconds().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose remainder is less confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

@@ -307,7 +307,7 @@ fn test_nanosecond_range() {
// Far beyond range
let maximum = "2262-04-11T23:47:16.854775804UTC";
let parsed: DateTime<Utc> = maximum.parse().unwrap();
let beyond_max = parsed + TimeDelta::days(365);
let beyond_max = parsed + Days::new(365);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe we should resurrect your work on CalendarDuration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to. Part 1 is ready for review 😄. Maybe wait a few more weeks until we are good underway with the Result stuff?

@@ -102,6 +102,10 @@ impl TimeDelta {
/// Panics when the duration is out of bounds.
#[inline]
#[must_use]
#[deprecated(
since = "0.4.35",
note = "Use `TimeDelta::try_weeks` instead or consider the `Days` type"
Copy link
Member

Choose a reason for hiding this comment

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

So I don't really want to recommend the Days type here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I'll remove it.

}

let date = try_opt!(self.date.checked_add_signed(TimeDelta::seconds(rhs)));
let (time, rhs_days) = self.time.overflowing_add_signed(rhs);
Copy link
Member

Choose a reason for hiding this comment

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

rhs_days -> remainder here as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, stupid!

@pitdicker pitdicker merged commit 9e667b6 into chronotope:main Mar 6, 2024
35 checks passed
@pitdicker pitdicker deleted the time_delta_cleanup branch March 6, 2024 13:04
HuijingHei added a commit to HuijingHei/bootupd that referenced this pull request Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate panicking TimeDelta initializers
2 participants