chronotope / chrono

Date and time library for Rust
Other
3.3k stars 523 forks source link

Saturating operations #1579

Open bragov4ik opened 4 months ago

bragov4ik commented 4 months ago

saturating_* methods might be quite useful. They are present in standard library for primitive types, so the use case can be quite frequent (?).

I found only one mention of saturation in this repo: "...library users could implement some saturating_* methods...".

Since there seems to be no separate discussion about this, I suggest to consider this implementation here.

bragov4ik commented 4 months ago

I encountered a case where I need to add Duration to NativeDate with saturation. There's a function that generates intervals of dates (NativeDate), with some step (Duration). It makes sense to have step = Duration::max_value() if the caller wants a single interval. Saturating add seemd quite helpful there.

pub fn generate_date_ranges(
    start: NaiveDate,
    end: NaiveDate,
    step: Duration,
) -> Vec<RangeInclusive<NaiveDate>> {
    let mut date_range = Vec::new();
    let mut next_start = start;
    while next_start < end {
        let next_end = next_start.saturating_add(step);
        date_range.push(RangeInclusive::new(next_start, next_end));
        next_start = next_end;
    }
    date_range
}
djc commented 4 months ago

A single person requesting this feature is not enough, in this case, to add this feature, I think. Let's keep this open and see if other people turn up that want/need this?

pitdicker commented 4 months ago

For integers saturation operations make sense because they have a commonly agreed-upon range. For dates and datetimes the range we support is somewhat arbitrary, and min and max are way outside the sensible range of dates. Saturating operations would make the min and max values more prominent, and I don't think that is good thing.

Veetaha commented 4 months ago

Hey @djc. I'm writing a code that needs to pass DateTime<Utc> as a filter to a DB request to specify a time range for the calculation to be limited to the past 30 days. I don't see any good way to handle the None case of Utc::now().checked_sub_days(chrono::Days::new(30)). I just want a date that is exactly 30 days from now (or lower if at the edge of timestamp starting points). How can I know if there were any time gaps so that I could skip them? How should I skip them? I think a saturating subtraction method would need to handle this.

Maybe I landed on a wrong issue, but the main comment is that I'd like a method that gracefully handles gaps in time and skips them when doing subtraction even if it doesn't saturate at the bounds of allowed date-time values. I can just use a default DateTime::MIN_UTC instead. In fact, I think checked_sub method should return a LocalResult instead of option so that I could act upon timestamps ambiguity or out-of-range errors but it should still handle gaps.

pitdicker commented 4 months ago

How can I know if there were any time gaps so that I could skip them?

If the time zone is UTC there are no gaps, Utc::now().checked_sub_days(chrono::Days::new(30)) will always succeed unless your pc clock is set 250.000+ years into the past. Or were you thinking of another cause?

If the time zone is Local or something else that supports DST it is best to work with methods that return LocalResult, en we will use it as the return value of more methods in chrono 0.5. Let me need if I should write an example.

djc commented 4 months ago

@pitdicker was thinking about this: if the result of a calculation is None, IIRC that means it falls into a gap. I read the previous comment as wondering if there was a way to get at the gap boundaries, which seems like a reasonable request. Do we have something like that today? (I don't think so.) How hard do you think that would be to add?

pitdicker commented 4 months ago

Local::now().naive_local().checked_sub_days(chrono::Days::new(30))?.and_local_timezone(Local) should do the same and return a LocalResult. earliest() and latest() can then be used for the gap boundaries.

Quoting from https://github.com/chronotope/chrono/issues/1448:

Methods on DateTime currently don't return a LocalResult, but consider any result that falls in a timezone transition as an error. This makes it cumbersome for users to handle these cases correctly. They have to work on a NaiveDateTime and then convert it back with TimeZone::from_local_datetime. Returning a LocalResult would be a big improvement for correctness in my opinion.

Sorry for being absent again. I really need to continue working on this for 0.5...

Veetaha commented 4 months ago

If the time zone is UTC there are no gaps

Oh, right... I was confused by the method signature and docs mentioning gaps. It is indeed basically infallible in UTC, so there is no problem in my case

djc commented 4 months ago

Local::now().naive_local().checked_sub_days(chrono::Days::new(30))?.and_local_timezone(Local) should do the same and return a LocalResult. earliest() and latest() can then be used for the gap boundaries.

earliest() and latest() don't actually return gap boundaries, right? They only yield a value for Single and Ambiguous which I suppose don't apply here.