Deprecate panicking constructors of `TimeDelta` #1450

pitdicker · 2024-02-21T06:35:50Z

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.

codecov · 2024-02-21T06:39:09Z

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.

djc · 2024-03-06T10:18:50Z

src/naive/date/mod.rs

@@ -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")


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

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".

That works for me.

djc · 2024-03-06T10:20:26Z

src/naive/datetime/mod.rs

-
-        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));


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

I suppose remainder is less confusing.

Sounds good.

djc · 2024-03-06T10:21:17Z

src/datetime/tests.rs

@@ -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);


Hmm, maybe we should resurrect your work on CalendarDuration?

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?

djc · 2024-03-06T10:22:26Z

src/time_delta.rs

@@ -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"


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

Okay, I'll remove it.

djc · 2024-03-06T12:55:31Z

src/naive/datetime/mod.rs

-        }
-
-        let date = try_opt!(self.date.checked_add_signed(TimeDelta::seconds(rhs)));
+        let (time, rhs_days) = self.time.overflowing_add_signed(rhs);


rhs_days -> remainder here as well?

Oops, stupid!

Xerf to chronotope/chrono#1450

pitdicker force-pushed the time_delta_cleanup branch 4 times, most recently from 578771b to cf8765e Compare February 22, 2024 12:47

pitdicker marked this pull request as ready for review February 22, 2024 12:53

pitdicker changed the title ~~Prepare TimeDelta for deprecating panicking constructors~~ Deprecate panicking constructors of TimeDelta Feb 22, 2024

pitdicker mentioned this pull request Feb 29, 2024

Deprecate timestamp methods on NaiveDateTime #1473

Merged

pitdicker added 3 commits March 1, 2024 11:55

Documentation fixes

67427fc

Use checked_add_days in NaiveDateWeeksIterator

abec3cd

Use simplified resolve_week_date

1a5e7f6

pitdicker force-pushed the time_delta_cleanup branch from cf8765e to 006d58d Compare March 1, 2024 11:05

djc reviewed Mar 6, 2024

View reviewed changes

Replace TimeDelta::days with try_days

679a2f8

pitdicker force-pushed the time_delta_cleanup branch from 006d58d to 6a1be47 Compare March 6, 2024 12:26

djc approved these changes Mar 6, 2024

View reviewed changes

pitdicker added 9 commits March 6, 2024 13:58

Replace TimeDelta::seconds with try_seconds

0dc7353

Tests: use Days type when it is more appropriate than TimeDelta

09873ec

Tests: replace TimeDelta::weeks with try_weeks

f5c9644

Tests: replace TimeDelta::days with try_days

f4bf70e

Tests: replace TimeDelta::hours with try_hours

1a49b49

Tests: replace TimeDelta::minutes with try_minutes

786123e

Tests: replace TimeDelta::seconds with try_seconds

6543d02

Tests: replace TimeDelta::milliseconds with try_milliseconds

422cd76

Deprecate panicking TimeDelta constructors

563c53d

pitdicker force-pushed the time_delta_cleanup branch from 6a1be47 to 563c53d Compare March 6, 2024 12:59

pitdicker merged commit 9e667b6 into chronotope:main Mar 6, 2024
35 checks passed

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

deps-update: bump chrono from 0.4.34 to 0.4.35

ac8dd79

Xerf to chronotope/chrono#1450

HuijingHei mentioned this pull request Mar 14, 2024

deps-update: bump chrono from 0.4.34 to 0.4.35 coreos/bootupd#629

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate panicking constructors of `TimeDelta` #1450

Deprecate panicking constructors of `TimeDelta` #1450

pitdicker commented Feb 21, 2024 •

edited

codecov bot commented Feb 21, 2024 •

edited

djc Mar 6, 2024

pitdicker Mar 6, 2024 •

edited

djc Mar 6, 2024

djc Mar 6, 2024

pitdicker Mar 6, 2024

djc Mar 6, 2024

djc Mar 6, 2024

pitdicker Mar 6, 2024

djc Mar 6, 2024

pitdicker Mar 6, 2024

djc Mar 6, 2024

pitdicker Mar 6, 2024

Deprecate panicking constructors of TimeDelta #1450

Deprecate panicking constructors of TimeDelta #1450

Conversation

pitdicker commented Feb 21, 2024 • edited

codecov bot commented Feb 21, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

pitdicker Mar 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deprecate panicking constructors of `TimeDelta` #1450

Deprecate panicking constructors of `TimeDelta` #1450

pitdicker commented Feb 21, 2024 •

edited

codecov bot commented Feb 21, 2024 •

edited

pitdicker Mar 6, 2024 •

edited