Skip to content

Conversation

ggoetz
Copy link
Contributor

@ggoetz ggoetz commented Feb 26, 2025

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

Copy link

codecov bot commented Feb 26, 2025

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.

@djc
Copy link
Member

djc commented Feb 26, 2025

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
Copy link
Contributor Author

ggoetz commented Feb 26, 2025

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.

/// 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
Copy link
Member

@djc djc Feb 27, 2025

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
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
35 checks passed
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