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

        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

    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

    #[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

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

cBournhonesque mentioned this pull request Oct 28, 2024

chrono pinned cBournhonesque/lightyear#681

Closed

Deprecate panicking constructors of TimeDelta #1450

Deprecate panicking constructors of TimeDelta #1450

Uh oh!

Conversation

pitdicker commented Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitdicker Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Deprecate panicking constructors of `TimeDelta` #1450

Deprecate panicking constructors of `TimeDelta` #1450

pitdicker commented Feb 21, 2024 •

edited

Loading

codecov bot commented Feb 21, 2024 •

edited

Loading

pitdicker Mar 6, 2024 •

edited

Loading