Create the subsec_millis and subsec_micros methods for Duration #1351
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
| } | ||
|
|
||
| /// Returns the number of milliseconds such that | ||
| /// `subsec_millis() + num_seconds() * MILLIS_PER_SEC` is the total number of |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I find it a bit strange that some of them are
i32and some arei64
I believe the goal for using i32 is to lessen resource "footprint" chrono objects.
| @@ -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 | |||
There was a problem hiding this comment.
@pitdicker same question as below regarding pub MICROS_PER_SEC.
fbc176f to
4829b7f
Compare
4829b7f to
10cd25f
Compare
| @@ -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. | |||
There was a problem hiding this comment.
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.
| /// 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; |
There was a problem hiding this comment.
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).
|
@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 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. |
|
@botahamec Can you let us now if you are still interested in working on this PR? |
|
Closing due to inactivity. |
Fixes #1348