Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create the subsec_millis and subsec_micros methods for Duration #1351

Closed
wants to merge 3 commits into from
Closed

Create the subsec_millis and subsec_micros methods for Duration #1351

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2297696
Create the subsec_millis and subsec_micros methods for Duration
botahamec Nov 4, 2023
2a95dc5
Change the documentation for subsec methods
botahamec Nov 8, 2023
10cd25f
Move constants to `lib.rs`
botahamec Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/duration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

@djc djc Nov 20, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

///
/// This is the number of nanoseconds such that
/// `subsec_nanos() + num_seconds() * NANOS_PER_SEC` is the total number of
/// nanoseconds in the duration.
pub const fn subsec_nanos(&self) -> i32 {
Expand All @@ -232,14 +234,18 @@ impl Duration {
}
}

/// Returns the number of microseconds such that
/// Returns the number of microseconds in the fractional part of the duration.
///
/// This is the number of microseconds such that
/// `subsec_micros() + num_seconds() * MICROS_PER_SEC` is the total number of
botahamec marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pitdicker same question as below regarding pub MICROS_PER_SEC.

/// microseconds in the duration.
pub const fn subsec_micros(&self) -> i32 {
self.subsec_nanos() / NANOS_PER_MICRO
}

/// Returns the number of milliseconds such that
/// Returns the number of milliseconds in the fractional part of the duration.
///
/// This is the number of milliseconds such that
/// `subsec_millis() + num_seconds() * MILLIS_PER_SEC` is the total number of
Copy link
Contributor

@jtmoon79 jtmoon79 Nov 4, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@djc djc Nov 6, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jtmoon79 jtmoon79 Nov 16, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// milliseconds in the duration.
pub const fn subsec_millis(&self) -> i32 {
Expand Down