chrono icon indicating copy to clipboard operation
chrono copied to clipboard

Add `CalendarDuration` type

Open pitdicker opened this issue 2 years ago • 12 comments
trafficstars

See https://github.com/chronotope/chrono/issues/1282.

As a first PR this adds nothing but the type, methods to set and get the individual components, and a Display implementation.

We have 5 components: months, days, minutes, seconds and nanoseconds. Example of ways to initialize a CalendarDuration:

use chrono::CalendarDuration;
// Duration in calendar months and days.
CalendarDuration::new().months(15).days(5);
// Duration in calendar days, and 3 hours.
CalendarDuration::new().days(5).hms(3, 0, 0).unwrap();
// Duration that will precisely count the number of seconds, including leap seconds.
CalendarDuration::new().seconds(23_456).millis(789).unwrap();

As described in https://github.com/chronotope/chrono/issues/1282#issuecomment-1719937171 we squeeze both the minutes and seconds components into one u32. The idea is that it is too strange and niche to have two large components that only differ in how they count leap seconds. So we either have:

  • a large number of minutes, and sub-minute seconds (which works almost like ignoring leap-seconds exist)
  • a large number of minutes (which accurately count leap seconds)

The Debug implementation will format the type as if it is a struct with fields for 5 components.

pitdicker avatar Sep 14 '23 19:09 pitdicker

For initialization the methods work much like the builder pattern, except that we don't need a seperate builder type. We have methods to initialize the individual components as months(), days(), hms(), seconds() and nanos(). For convenience I added methods that initialize with other units, but with a name that hopefully helps to make clear on which component they act: years_and_months(), weeks_and_days(), micros() and millis().

I have intentionally added only methods that set a field, not methods that add some value to a field. And also not Add implementations. It is quite easy to create a CalendarDuration with a combination of components that will be near-impossible to explain. In my opinion the type should not be treated as black boxes that you can just add together. You should be aware of the existing components when changing anything.

To get the value of the individual components, we have not five but four methods (because minutes and seconds are closely related): months_component(), days_component(), mins_and_secs_component(), and nanos_component(). No convenience methods here.

pitdicker avatar Sep 14 '23 19:09 pitdicker

Codecov Report

Attention: Patch coverage is 92.24806% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 91.81%. Comparing base (f8cecbe) to head (3739e2f).

Files Patch % Lines
src/calendar_duration.rs 92.24% 20 Missing :warning:
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #1290    +/-   ##
========================================
  Coverage   91.80%   91.81%            
========================================
  Files          37       38     +1     
  Lines       18151    18409   +258     
========================================
+ Hits        16664    16902   +238     
- Misses       1487     1507    +20     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 14 '23 19:09 codecov[bot]

(Please avoid adding more stuff to this PR -- other than per review feedback.)

djc avatar Sep 15 '23 10:09 djc

Setting to draft for now. I have most of the parser ready, but need to clean up things a bit more before it is ready a second look.

pitdicker avatar Sep 17 '23 18:09 pitdicker

(Please avoid adding more stuff to this PR -- other than per review feedback.)

Sorry. I added two more things, but minor I think: #[must_use] to a few methods, and a test to check the setters correctly work with our-of-range values.

pitdicker avatar Sep 18 '23 11:09 pitdicker

Started working on this again. @djc are you sure you don't want to have the parsing code split out from this PR so we can merge a minimal part first?

pitdicker avatar Mar 23 '24 14:03 pitdicker

Put a couple of hours into self-review and improving the initial documentation.

pitdicker avatar Mar 24 '24 11:03 pitdicker

@djc You gave an approval, but want to have a final look?

pitdicker avatar Apr 07 '24 11:04 pitdicker