Create the subsec_millis and subsec_micros methods for Duration #1351

botahamec · 2023-11-04T20:11:11Z

codecov · 2023-11-04T20:16:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (15c8885) 91.61% compared to head (10cd25f) 91.63%.
Report is 3 commits behind head on 0.4.x.

Additional details and impacted files

@@            Coverage Diff             @@
##            0.4.x    #1351      +/-   ##
==========================================
+ Coverage   91.61%   91.63%   +0.01%     
==========================================
  Files          38       38              
  Lines       17482    17519      +37     
==========================================
+ Hits        16016    16053      +37     
  Misses       1466     1466

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

src/duration.rs

jtmoon79 · 2023-11-04T20:43:10Z

src/duration.rs

+    }
+
+    /// Returns the number of milliseconds such that
+    /// `subsec_millis() + num_seconds() * MILLIS_PER_SEC` is the total number of


I like this docstring specificity.

@pitdicker should MILLIS_PER_SEC be made pub? If a user wants to follow this docstring implied suggestion (compute subsec_millis() + num_seconds() * MILLIS_PER_SEC) then they'll want to access MILLIS_PER_SEC. This docstring is taken from subsec_nanos docstring which has the same implied suggestion but using NANOS_PER_SEC, so NANOS_PER_SEC should probably also be pub.

Stretch goal would be to make all the constants in that clump of constants above into pub.

I think it makes sense to make these constants part of the public API.

For these docstrings, I'd like to keep a shorter first line, more like core (which has "Returns the fractional part of this Duration, in whole microseconds."). We can still show this formula, but it shouldn't be in the first line.

A made a change like that to the docstrings. I'm wondering if those constants should stay in the duration module. They might make more sense if they were put in the lib.rs. I can't imagine anyone looking for these constants intuiting that they'd be in duration.

Makes sense to me!

Just made that change, but I do have one more question. Now that these are public, I find it a bit strange that some of them are i32 and some are i64. I think this makes sense for what we're using them for, and if it's useful for us to have them that way, then it's probably useful for someone else too, but I wanted to bring it in case someone thinks we should make it more consistent.

I find it a bit strange that some of them are i32 and some are i64

I believe the goal for using i32 is to lessen resource "footprint" chrono objects.

jtmoon79 · 2023-11-04T20:43:39Z

src/duration.rs

@@ -232,6 +232,20 @@ impl Duration {
        }
    }

+    /// Returns the number of microseconds such that
+    /// `subsec_micros() + num_seconds() * MICROS_PER_SEC` is the total number of


@pitdicker same question as below regarding pub MICROS_PER_SEC.

djc

I'd like this split into clean commits.

One for moving the constants (and making them pub)
One for adding new methods (with complete docs and tests)
One for improving docs on current methods

jtmoon79

LGTM with nitpicks about inconsistent docstring

src/lib.rs

botahamec · 2023-11-19T14:13:52Z

Sorry I took a while
@jtmoon79 I addressed your comments and rebased to make my commits match @djc's suggestion

src/lib.rs

djc · 2023-11-20T09:45:46Z

src/duration.rs

@@ -221,7 +221,9 @@ impl Duration {
        }
    }

-    /// Returns the number of nanoseconds such that
+    /// Returns the number of nanoseconds in the fractional part of the duration.


These improvements for the new methods should be introduced in the same commit as the methods themselves, as I explained in my previous comment. Only the changes for existing methods should remain in this commit.

pitdicker · 2024-01-30T16:34:42Z

src/lib.rs

+/// The number of nanoseconds in a second.
+pub const NANOS_PER_SEC: i32 = 1_000_000_000;
+/// The number of microseconds in a second.
+pub const MICROS_PER_SEC: i64 = 1_000_000;


The types of these constants are 'whatever needs the fewest casts in our current code'.

That is: everything that has to do with seconds or more is using the type of the Duration.secs field.
Everything that has to do with subsecond units uses the type of the Duration.nanos field.
For most constants that seems like a good default.

NANOS_PER_SEC and MICROS_PER_SEC are at the boundary. NANOS_PER_SEC is currently an i32, and we sometimes cast it to an i64.

Maybe we should change MICROS_PER_SEC to also be an i32. Argument is consistency with NANOS_PER_SEC, and that widening casts are infallible and available with a From impl (even though this is just a constant and would not fail anyway).

pitdicker · 2024-01-30T16:38:21Z

@botahamec Do you want to push this PR over the finish line? Please adjust the commits a bit more matching @djc's request, and adjust the type of MICROS_PER_SEC.

Note that we will hopefully merge two PRs the next few days that touch surrounding code: #1337 and #1385. You may want to wait with rebasing until that is done.

pitdicker · 2024-02-02T07:35:23Z

Note that we will hopefully merge two PRs the next few days that touch surrounding code: #1337 and #1385. You may want to wait with rebasing until that is done.

They are merged now.

pitdicker · 2024-02-21T16:17:47Z

@botahamec Can you let us now if you are still interested in working on this PR?

pitdicker · 2024-03-08T10:46:40Z

Closing due to inactivity.

jtmoon79 suggested changes Nov 4, 2023

View reviewed changes

src/duration.rs Show resolved Hide resolved

src/duration.rs Outdated Show resolved Hide resolved

jtmoon79 reviewed Nov 4, 2023

View reviewed changes

djc reviewed Nov 15, 2023

View reviewed changes

jtmoon79 approved these changes Nov 16, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

jtmoon79 mentioned this pull request Nov 16, 2023

Add from_timestamp_nanos #1357

Merged

botahamec added 2 commits November 19, 2023 09:10

Create the subsec_millis and subsec_micros methods for Duration

2297696

Change the documentation for subsec methods

2a95dc5

botahamec force-pushed the subsec-millis-micros branch from fbc176f to 4829b7f Compare November 19, 2023 14:12

jtmoon79 approved these changes Nov 19, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

jtmoon79 approved these changes Nov 19, 2023

View reviewed changes

Move constants to lib.rs

10cd25f

botahamec force-pushed the subsec-millis-micros branch from 4829b7f to 10cd25f Compare November 20, 2023 01:48

djc reviewed Nov 20, 2023

View reviewed changes

pitdicker reviewed Jan 30, 2024

View reviewed changes

pitdicker mentioned this pull request Feb 2, 2024

Make remaining methods const #1337

Merged

pitdicker closed this Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create the subsec_millis and subsec_micros methods for Duration #1351

Create the subsec_millis and subsec_micros methods for Duration #1351

botahamec commented Nov 4, 2023

codecov bot commented Nov 4, 2023 •

edited

jtmoon79 Nov 4, 2023 •

edited

djc Nov 6, 2023 •

edited

botahamec Nov 8, 2023

djc Nov 9, 2023

botahamec Nov 15, 2023

jtmoon79 Nov 16, 2023 •

edited

jtmoon79 Nov 4, 2023

djc left a comment

jtmoon79 left a comment

botahamec commented Nov 19, 2023

djc Nov 20, 2023 •

edited

pitdicker Jan 30, 2024 •

edited

pitdicker commented Jan 30, 2024

pitdicker commented Feb 2, 2024

pitdicker commented Feb 21, 2024

pitdicker commented Mar 8, 2024

Create the subsec_millis and subsec_micros methods for Duration #1351

Create the subsec_millis and subsec_micros methods for Duration #1351

Conversation

botahamec commented Nov 4, 2023

codecov bot commented Nov 4, 2023 • edited

Codecov Report

jtmoon79 Nov 4, 2023 • edited

Choose a reason for hiding this comment

djc Nov 6, 2023 • edited

Choose a reason for hiding this comment

botahamec Nov 8, 2023

Choose a reason for hiding this comment

djc Nov 9, 2023

Choose a reason for hiding this comment

botahamec Nov 15, 2023

Choose a reason for hiding this comment

jtmoon79 Nov 16, 2023 • edited

Choose a reason for hiding this comment

jtmoon79 Nov 4, 2023

Choose a reason for hiding this comment

djc left a comment

Choose a reason for hiding this comment

jtmoon79 left a comment

Choose a reason for hiding this comment

botahamec commented Nov 19, 2023

djc Nov 20, 2023 • edited

Choose a reason for hiding this comment

pitdicker Jan 30, 2024 • edited

Choose a reason for hiding this comment

pitdicker commented Jan 30, 2024

pitdicker commented Feb 2, 2024

pitdicker commented Feb 21, 2024

pitdicker commented Mar 8, 2024

codecov bot commented Nov 4, 2023 •

edited

jtmoon79 Nov 4, 2023 •

edited

djc Nov 6, 2023 •

edited

jtmoon79 Nov 16, 2023 •

edited

djc Nov 20, 2023 •

edited

pitdicker Jan 30, 2024 •

edited