Skip to content

Add subsec_micros and subsec_millis methods to TimeDelta - #1668

Merged
djc merged 2 commits into
chronotope:mainfrom
ggoetz:subsec-millis-micros
Feb 27, 2025
Merged

Add subsec_micros and subsec_millis methods to TimeDelta#1668
djc merged 2 commits into
chronotope:mainfrom
ggoetz:subsec-millis-micros

Conversation

@ggoetz

@ggoetz ggoetz commented Feb 26, 2025

Copy link
Copy Markdown
Contributor

This PR revives @botahamec 's PR #1351, which has been abandoned, and adds subsec_micros and subsec_millis to the public API for chrono::TimeDelta, for consistency with the API of std::time::Duration.

Summary of changes:

  • update the docstring for TimeDelta::subsec_nanos() and add a test for the method.
  • add subsec_millis and subsec_micros, along with unit tests.

Fixes: #1348

@codecov

codecov Bot commented Feb 26, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.07%. Comparing base (2b7a28e) to head (0d4b934).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1668      +/-   ##
==========================================
+ Coverage   91.05%   91.07%   +0.01%     
==========================================
  Files          37       37              
  Lines       17402    17431      +29     
==========================================
+ Hits        15846    15875      +29     
  Misses       1556     1556              

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

Comment thread src/lib.rs Outdated
@djc

djc commented Feb 26, 2025

Copy link
Copy Markdown
Member

Honestly I'm not convinced by the argument that these consts should be made public just so two of them can make a single appearance in a docstring. IMO it would be fine to leave the constants alone and use literals to explain the methods.

@ggoetz

ggoetz commented Feb 26, 2025

Copy link
Copy Markdown
Contributor Author

Honestly I'm not convinced by the argument that these consts should be made public just so two of them can make a single appearance in a docstring. IMO it would be fine to leave the constants alone and use literals to explain the methods.

Makes sense to me -- and there was precedent with the previous docstring for subsec_nanos to only use a literal without exposing the corresponding const. I've reverted the commit that exposed the constants.

Comment thread src/time_delta.rs
/// Returns the number of nanoseconds such that
/// Returns the number of nanoseconds in the fractional part of the duration.
///
/// This is the number of nanoseconds such that

@djc djc Feb 27, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, by literal I meant to replace NANOS_PER_SEC with 1_000_000_000 here (and same for the new methods). Other than that this seems fine!

(Maybe also update the PR description to describe the current approach.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the 3 comments and the PR description.

ggoetz and others added 2 commits February 27, 2025 17:10
Co-authored-by: Micha White <botahamec@outlook.com>
Co-authored-by: Micha White <botahamec@outlook.com>
@djc
djc merged commit 042109f into chronotope:main Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add subsec methods to Duration

2 participants