Return error when rounding with a zero duration

[Dav1dde]

The division by zero (modulo) panics, duration_round already checks for 0 and returns the original value.

[Dav1dde]

codecov error seems unrelated

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.78%. Comparing base (7f57dde) to head (50ae60f).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1474   +/-   ##
=======================================
  Coverage   91.78%   91.78%           
=======================================
  Files          37       37           
  Lines       18167    18176    +9     
=======================================
+ Hits        16674    16683    +9     
  Misses       1493     1493           

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

[pitdicker]

@Dav1dde Thank you for fixing this!

My opinion of this module remains low 😞.

In theory rounding or truncating to multiples of 0 (nano)seconds doesn't make sense. Wouldn't it be better to return an error instead?

[Dav1dde]

In theory rounding or truncating to multiples of 0 (nano)seconds doesn't make sense. Wouldn't it be better to return an error instead?

In my usecase returning the original value is what I would need. Thinking about it a bit more, to get that same behaviour it should also be possible to truncate by the smallest represented unit in the timestamp (1 ns), which does make more sense.

But I think being consistent with duration_round is better than returning an error, even if it does probably make more sense.

[pitdicker]

If @djc agrees I would like to see both rounding and truncating return an error on zero, and leave it up to the caller to use sane arguments. I.e. make not only negative values return RoundingError::DurationExceedsLimit but also zero.

[djc]

@Dav1dde what are you using these methods for? What are you trying to achieve?

[Dav1dde]

@Dav1dde what are you using these methods for? What are you trying to achieve?

Aggregating data by timestamp with a given granularity.

Zero was just the first thing I reached for. Though 1ns would make technically more sense, it also requires you to know the minimum precision and in an interface that only accepts seconds 1ns is not possible to supply -> zero being the easy alternative.

While an error makes sense it is also a lot more annoying to deal with. You have to match exactly on the variant, then hope zero has it's own variant, or just check for zero before passing it duration_trunc.

[pitdicker]

If @djc agrees I would like to see both rounding and truncating return an error on zero, and leave it up to the caller to use sane arguments. I.e. make not only negative values return RoundingError::DurationExceedsLimit but also zero.

@Dav1dde Would you be willing to update this PR to return consistently return an error when rounding to multiples of zero?

In your situation if a function accepts 0 to signal no rounding I think the function should simply not call the chrono methods (with some minimum granularity) but return the input.

[Dav1dde]

@Dav1dde Would you be willing to update this PR to return consistently return an error when rounding to multiples of zero?

Sure, if that is the desired behaviour.

While overall the error makes more sense, I don't think that is a good idea. Because it is different behaviour to what duration_round does. A user replacing duration_round with duration_trunc will now get an error or the other way around instead of an error they get a value. I think the behaviour on chrono should be consistent here.

[pitdicker]

Yes, both should error.

[Dav1dde]

Yes, both should error.

Should I update both?

[pitdicker]

Please 😄

[Dav1dde]

@pitdicker updated! Also added tests for negative values.

Edit: Any changelog I should update?

[pitdicker]

Thank you, looks good!

We add release notes to GitHub releases, nothing you need to update.

[pitdicker]

Interesting. Another weirdness in this module. 'will fail if TimeDelta is bigger than the timestamp.' So rounding to hours will fail for values within the first hour of 1970-01-01 😞. Don't worry about it for this PR though.

[pitdicker]

@djc Do you have time to look at this PR?

[djc]

Will have a look tomorrow.

[djc]

LGTM (but let's squash during merge?).