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.

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

Add rounding and truncating by Duration #445

Merged

Conversation

robyoung
Copy link
Contributor

@robyoung robyoung commented Jul 2, 2020

Resolves #280

This adds a DurationRound trait modelled after SubsecRound. It turned out to be a bit more complex that I had anticipated to handle all possible DateTimes and Durations so I chose not to.

Both rounding and truncating are done via Duration::num_nanoseconds and DateTime::timestamp_nanos. This means that they will fail if either the Duration or the DateTime are too big to represented as nanoseconds. They will also fail if the Duration is bigger than the timestamp.

@robyoung robyoung force-pushed the feature/add-round-trunc-by-duration branch 4 times, most recently from 8b10958 to 796422f Compare July 2, 2020 20:40
Copy link
Contributor

@quodlibetor quodlibetor left a comment

Choose a reason for hiding this comment

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

Thanks for this! I'm going to take a look at the logic when I'm a bit less tired.

src/round.rs Outdated Show resolved Hide resolved
@robyoung robyoung force-pushed the feature/add-round-trunc-by-duration branch from 359c0a7 to 540ad01 Compare July 3, 2020 08:44
@robyoung
Copy link
Contributor Author

robyoung commented Jul 3, 2020

I've just squashed the little fixup commits I did on my phone last night.

src/round.rs Outdated
/// [`DateTime::timestamp_nanos`]. This means that they will fail if either the
/// `Duration` or the `DateTime` are too big to represented as nanoseconds. They
/// will also fail if the `Duration` is bigger than the timestamp.
pub trait DurationRound {
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall this trait looks great, thanks!

A couple notes, though:

  • I think that it should take an associated type for errors instead of panicking
  • The methods should follow the same name order as the trait

Leading to:

pub trait DurationRound {

    type Err: std::error::Error;

    fn duration_round(&self, duration: Duration) -> Result<Self, Self::Err>;
    fn duration_trunc(&self, duration: Duration) -> Result<Self, Self::Err>;
}

There are possibly other naming schemes that we could consider:

  • trait RoundToDuration gives fn round_to_duration and fn trunc_to_duration which is maybe
    worse.

  • trait RoundTo<T> gives some worse ergonomics in some cases, but it may allow us to do something
    like:

    struct Seconds;
    
    impl<Tz> RoundTo<Seconds> for DateTime<Tz> { .. }

    which would remove the limitation of dealing with nanoseconds.

Actually, could we remove the nanoseconds limitation in specific cases by checking what the
smallest precision in the duration is? Even just deciding to do seconds vs nanoseconds by checking
if Duration.nanos_mod_sec() is non-zero should dramatically reduce the cases where this
could error.

I think leaving this as DurationRound but adding the check for precision is probably the way to go, but I'm curious what your use-case is, and if that makes sense for it?

Copy link
Contributor Author

@robyoung robyoung Jul 4, 2020

Choose a reason for hiding this comment

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

A couple notes, though:

  • I think that it should take an associated type for errors instead of panicking
  • The methods should follow the same name order as the trait

Totally agree. I've taken a stab at it.

Actually, could we remove the nanoseconds limitation in specific cases by checking what the smallest precision in the duration is? Even just deciding to do seconds vs nanoseconds by checking if Duration.nanos_mod_sec() is non-zero should dramatically reduce the cases where this could error.

Yes. I had tried this originally but found that Duration.nanos_mod_secs is private. It then took me quite a while to discover that Duration sometimes comes from the time crate so I couldn't even make it crate public. Can you think of a way around that? I thought about converting to std::time::Duration and then looking at subsec_nanos but then it's not available on no_std.

but I'm curious what your use-case is, and if that makes sense for it?

My use case is pretty simple. Bucketing events that happened at most a year ago into durations of between a minute and a week. I will never come close to the limitations. The RoundTo<T> trait would be a little less easy to work with for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had tried this originally but found that Duration.nanos_mod_secs is private.

Ah. Yeah this will be resolved by #286 , which is continuously becoming higher priority and I'm going to make time for.

For now we can just accept the limited range (pretty sure it's still 1970±500 years) and file an issue to expand it either when it's a problem for folks or when I finally finish that.

robyoung added a commit to robyoung/chrono that referenced this pull request Jul 4, 2020
robyoung added a commit to robyoung/chrono that referenced this pull request Jul 4, 2020
@robyoung robyoung force-pushed the feature/add-round-trunc-by-duration branch from ec19b8a to bbeb0cd Compare July 4, 2020 13:00
robyoung added a commit to robyoung/chrono that referenced this pull request Jul 4, 2020
@robyoung robyoung force-pushed the feature/add-round-trunc-by-duration branch from 5fe6361 to f87fe2d Compare July 4, 2020 13:13
robyoung added a commit to robyoung/chrono that referenced this pull request Jul 4, 2020
@robyoung robyoung force-pushed the feature/add-round-trunc-by-duration branch from f87fe2d to e08c779 Compare July 4, 2020 14:15
src/round.rs Outdated
Comment on lines 108 to 109
#[cfg(not(any(feature = "std", test)))]
type Err;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's probably worth adding a trait bound here of Debug + Display, anyhow uses that bound in no std so it seems not insane. We can compatibly remove trait bounds down the line, but not add them, so I'd rather be as conservative as possible here, but I'm not familiar with no_std in general.

Copy link
Contributor

@quodlibetor quodlibetor left a comment

Choose a reason for hiding this comment

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

This is looking great! A couple requests for docs/tests and I think that's it. Thanks!

src/round.rs Outdated Show resolved Hide resolved
src/round.rs Outdated Show resolved Hide resolved
src/round.rs Outdated Show resolved Hide resolved
robyoung added a commit to robyoung/chrono that referenced this pull request Jul 4, 2020
@robyoung robyoung force-pushed the feature/add-round-trunc-by-duration branch from e08c779 to 7839d61 Compare July 4, 2020 16:31
robyoung added a commit to robyoung/chrono that referenced this pull request Jul 4, 2020
@robyoung robyoung force-pushed the feature/add-round-trunc-by-duration branch 2 times, most recently from 71d7caf to eff8995 Compare July 4, 2020 18:21
@robyoung robyoung force-pushed the feature/add-round-trunc-by-duration branch from 6f96e6d to ddf24b9 Compare July 4, 2020 18:44
@quodlibetor quodlibetor merged commit 4348cd1 into chronotope:master Jul 5, 2020
@quodlibetor
Copy link
Contributor

Thanks!

@Tom1380
Copy link

Tom1380 commented Oct 27, 2020

So how do you truncate NaiveDateTime? Looking at the docs, I only see DateTime<Tz> as an implementor for DurationRound.

robyoung added a commit to robyoung/chrono that referenced this pull request Nov 30, 2020
pickfire pushed a commit to pickfire/chrono that referenced this pull request Jan 22, 2022
botahamec pushed a commit to botahamec/chrono that referenced this pull request May 26, 2022
pickfire pushed a commit to pickfire/chrono that referenced this pull request Jul 5, 2022
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 round/truncate functions for any specified Duration
3 participants