Duration features by pitdicker · Pull Request #1137 · chronotope/chrono

pitdicker · 2023-06-08T08:14:12Z

If we drop time 0.1 compatibility in #1095 it would be nice if we could also take care of some of the wishlist items for Duration.

Derive Default: Consider adding a "Default" implementation to Duration #717
Add a subsec_nanos method: Provide access to subsecond nanos in the public interface of Duration. #154
Add try_* builders: Fix panic in DateTime::checked_add_days #941
Implement AddAssign and SubAssign: [Feature Request] AddAssign/SubAssign implementation to chrono::Duration #1054

Three features that would be more involved and not part of this PR are:

Parsing: [Feature Request] Support parsing ISO 8601 Duration strings #579
Format Duration: format a Duration #197
Serde: Add Serialize/Deserialize for Duration #117

Making the remaining methods const (#309, #638) would depend on raising the MSRV as in #1080.

Based on #1095.

pitdicker · 2023-06-08T08:16:08Z

Maybe we can fix the confusion with std::Duration and our current OldDuration name in the documentation?
Rename Duration to TimeDelta, and make Duration a type alias.

jtmoon79 · 2023-07-09T05:34:54Z

Maybe we can fix the confusion with std::Duration and our current OldDuration name in the documentation? Rename Duration to TimeDelta, and make Duration a type alias.

Might be too late to chime in on this, but ChronoDuration is readily self-explanatory.

pitdicker · 2023-07-09T17:47:07Z

If a rename/alias to avoid confusion with the standard library is up for discussion:

ChonoDuration
TimeDelta (currently on main)
TimeSpan
Interval

@jtmoon79 I may not get the subtleties of the words completely. Is there one that is most appropriate for a value that can be positive and negative?

jtmoon79 · 2023-07-09T18:13:32Z

If a rename/alias to avoid confusion with the standard library is up for discussion:

ChonoDuration

TimeDelta (currently on main)

TimeSpan

Interval

@jtmoon79 I may not get the subtleties of the words completely. Is there one that is most appropriate for a value that can be positive and negative?

I think all the terms proposed there are sensible for positive and negative values.

I strongly prefer ChronoDuration because it readily explains itself as:

a Duration, similar to std::Duration
from the chrono crate

Synonyms like TimeDelta, TimeSpan, etc. imply the object is very different from std::Duration, and may add difficulty for non-native English readers.

fsktom · 2023-07-25T20:23:06Z

I'm very excited about these quality of life improvements to the duration type! (esp. finally having the iter::Sum implementation)

I have to agree with @jtmoon79 on the new name for Duration.
ChronoDuration really seems like the most sensible option. The other two sound too scientific/abstract in my opinion and may really make it seem like it's a lot different from std::time::Duration.
I think it's a better name, even if it makes it a little bit more verbose and the following would look pretty weird (consecutive chrono):

let dur = chrono::ChronoDuration::zero();

Though I suppose most people would do use chrono::ChronoDuration if they're dealing with time spans anyway.

pitdicker · 2023-07-27T14:40:35Z

Added a commit to make the methods const.

jtmoon79

I strongly prefer using alias ChronoDuration, e.g.

use crate::duration::Duration as ChronoDuration;

However, using just Duration is acceptable so long as it is consistent everywhere, e.g.

use crate::duration::Duration;

src/date.rs

jtmoon79 · 2023-08-05T05:00:43Z

src/duration.rs

+    }
+
+    /// Makes a new `Duration` with given number of seconds.
+    /// Returns None when the duration is more than `i64::MAX` milliseconds


Nitpick: docstring referring to specific value None (None) should format it as code, i.e. add backticks

/// Returns `None` when the duration is ...

jtmoon79 · 2023-08-05T05:02:32Z

src/duration.rs

        Ok(StdDuration::new(self.secs as u64, self.nanos as u32))
    }
+
+    /// Returns true if and only if d < MIN || d > MAX


nitpick: why use the mathematical proof phrase "if and only if" here? I suggest wording "if", i.e.

/// Returns `true` if d < MIN || d > MAX

codecov · 2023-08-28T08:30:57Z

Codecov Report

Merging #1137 (ae3827c) into 0.4.x (0f19d6b) will increase coverage by 0.02%.
The diff coverage is 98.75%.

@@            Coverage Diff             @@
##            0.4.x    #1137      +/-   ##
==========================================
+ Coverage   91.55%   91.57%   +0.02%     
==========================================
  Files          38       38              
  Lines       17355    17385      +30     
==========================================
+ Hits        15889    15920      +31     
+ Misses       1466     1465       -1

Files	Coverage Δ
src/naive/time/mod.rs	`96.55% <100.00%> (ø)`
src/duration.rs	`93.54% <98.71%> (+0.61%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

djc · 2023-09-08T07:39:01Z

Going to hold off on this for the next week, let's discuss again around Sep 18.

pitdicker · 2023-09-21T19:17:38Z

Would you like me to split up this PR?

djc · 2023-09-21T19:32:36Z

Yes, let's try it. Please rate limit your submissions!

pitdicker · 2023-09-21T19:35:16Z

Haha. Okay.

pitdicker · 2023-09-29T09:13:26Z

With #1327 merged this now only has the changes to make methods on Duration const. Partially based on #638.

djc · 2023-09-29T09:37:20Z

src/duration.rs

    }
+
+    /// Returns `true` if d < MIN || d > MAX
+    const fn out_of_bounds(&self) -> bool {


This should be a constructor, not a method on an already constructed type.

You are completely right!

Actually I wanted to have a Duration::new in #1310 and #1290.

pitdicker · 2024-01-26T20:52:08Z

The remainder of this PR has become part of #1337.

pitdicker force-pushed the duration_features branch from 564bfc8 to f34af09 Compare June 8, 2023 08:23

djc added the waiting-on-review label Jun 8, 2023

pitdicker force-pushed the duration_features branch 2 times, most recently from 40ee6f2 to 6da23f4 Compare June 8, 2023 17:17

pitdicker force-pushed the duration_features branch 2 times, most recently from 89e8b41 to ee4ebe3 Compare June 30, 2023 11:31

pitdicker mentioned this pull request Jul 17, 2023

Fix panic in DateTime::checked_add_days #941

Closed

pitdicker force-pushed the duration_features branch 2 times, most recently from 6da3232 to fdaa314 Compare July 18, 2023 20:44

pitdicker force-pushed the duration_features branch from fdaa314 to 7b97dfc Compare July 27, 2023 14:39

pitdicker force-pushed the duration_features branch 2 times, most recently from ba8dae7 to 262bdf5 Compare July 27, 2023 14:44

jtmoon79 suggested changes Aug 5, 2023

View reviewed changes

pitdicker mentioned this pull request Aug 5, 2023

Remove time 0.1 as optional dependency #1095

Merged

pitdicker force-pushed the duration_features branch 2 times, most recently from f17d78f to 986c25f Compare August 5, 2023 19:03

pitdicker force-pushed the duration_features branch from 986c25f to cf44a6c Compare August 28, 2023 08:27

pitdicker force-pushed the duration_features branch 2 times, most recently from fb90ec0 to 6799e82 Compare August 29, 2023 13:30

pitdicker force-pushed the duration_features branch from 6799e82 to 30ed074 Compare September 7, 2023 20:13

This was referenced Sep 17, 2023

^ the trait Default is not implemented for chrono::Duration #1297

Closed

Making Duration::seconds and similar functions const #638

Closed

pitdicker mentioned this pull request Sep 27, 2023

Duration features part 1 #1327

Merged

pitdicker force-pushed the duration_features branch from 30ed074 to 56e3691 Compare September 29, 2023 09:00

djc reviewed Sep 29, 2023

View reviewed changes

pitdicker force-pushed the duration_features branch from 56e3691 to 9edb340 Compare September 29, 2023 10:55

pitdicker and others added 4 commits October 1, 2023 21:58

Remove duplicate try_opt macro

d847576

Add Duration::new

afc42ea

Make all methods on Duration const

b4621ed

Add test_duration_const()

ae3827c

pitdicker force-pushed the duration_features branch from 9edb340 to ae3827c Compare October 2, 2023 10:17

This was referenced Oct 2, 2023

Make remaining methods const #1337

Merged

Provide access to subsecond nanos in the public interface of Duration. #154

Closed

pitdicker closed this Jan 26, 2024

pitdicker deleted the duration_features branch January 26, 2024 20:52

pitdicker mentioned this pull request Feb 5, 2024

Rename Duration to TimeDelta, add type alias #1406

Merged

Conversation

pitdicker commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitdicker commented Jun 8, 2023

Uh oh!

jtmoon79 commented Jul 9, 2023

Uh oh!

pitdicker commented Jul 9, 2023

Uh oh!

jtmoon79 commented Jul 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fsktom commented Jul 25, 2023

Uh oh!

pitdicker commented Jul 27, 2023

Uh oh!

jtmoon79 left a comment • 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!

Uh oh!

jtmoon79 Aug 5, 2023

Choose a reason for hiding this comment

Uh oh!

jtmoon79 Aug 5, 2023

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

djc commented Sep 8, 2023

Uh oh!

pitdicker commented Sep 21, 2023

Uh oh!

djc commented Sep 21, 2023

Uh oh!

pitdicker commented Sep 21, 2023

Uh oh!

pitdicker commented Sep 29, 2023

Uh oh!

djc Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

pitdicker Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

pitdicker commented Jan 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pitdicker commented Jun 8, 2023 •

edited

Loading

jtmoon79 commented Jul 9, 2023 •

edited

Loading

jtmoon79 left a comment •

edited

Loading

codecov bot commented Aug 28, 2023 •

edited

Loading