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

There is no way to add a "day" to a DateTime #339

Closed
sparky8251 opened this issue Sep 4, 2019 · 9 comments · Fixed by #784
Closed

There is no way to add a "day" to a DateTime #339

sparky8251 opened this issue Sep 4, 2019 · 9 comments · Fixed by #784

Comments

@sparky8251
Copy link

Just like in #318, this sample code can be used to explain the problem.

use chrono::NaiveDateTime;
use chrono::TimeZone;
use chrono::{DateTime, Duration};
use chrono_tz::US::Eastern;
use chrono_tz::Tz;

fn main() {
    let timestamp = 1572580800;

    let mut dt: DateTime<Tz> = Eastern.from_utc_datetime(&NaiveDateTime::from_timestamp(timestamp, 0));

    println!("Written datetime: {}", dt);

    //increment past the daylight savings change

    dt = dt + Duration::days(3);

    println!("Written datetime: {}", dt);
}

When it's run, the output is:

Written datetime: 2019-11-01 00:00:00 EDT
Written datetime: 2019-11-03 23:00:00 EST

What I would expect adding 3 days is that I would get 2019-11-4 00:00:00 EST. I didn't add 86400 * 3 to the time, I added Duration::days(3) so this feels inconsistent as a consumer of this crate.

I don't know if this belongs here or if it belongs in something like a chrono-utils crate, but I do feel it needs addressing or at least some sort of call out in the docs that it's not actually days. It's non-obvious that it's just an abstraction for "number of seconds in a day * X".

@quodlibetor quodlibetor changed the title Adding a day doesn't always add a day There is no way to add a "day" to a DateTime Sep 4, 2019
@frondeus
Copy link

frondeus commented Dec 2, 2019

Hi,

I noticed this problem appears only in newest versions of chrono:

I wrote this test:

#[cfg(test)]
mod tests {
    extern crate chrono_tz;

    use self::chrono_tz::Europe::Warsaw;

    #[test]
    fn test_chrono_add() { // https://github.com/chronotope/chrono/issues/339
        use std::ops::Add;
        use chrono::*;

        let timezone = Warsaw;
        let mut date = timezone.from_utc_datetime(&"2017-10-28T22:00:00Z".parse::<DateTime<Utc>>().unwrap().naive_utc());

        dbg!(&date);
        assert_eq!("2017-10-29", &date.format("%Y-%m-%d").to_string());
        date = date.add(Duration::days(1));
        assert_eq!("2017-10-30", &date.format("%Y-%m-%d").to_string());
        date = date.add(Duration::days(1));
        assert_eq!("2017-10-31", &date.format("%Y-%m-%d").to_string());
    }
}

With chrono = "=0.4.10" it's failing - "Expected 2017-10-30, found 2017-10-29"
While with chrono = "=0.4.7" it's working correctly

@frondeus
Copy link

frondeus commented Dec 2, 2019

2017-10-29T00:00:00CEST + 1 day = 2017-10-29T23:00:00:00CET

@quodlibetor
Copy link
Contributor

@frondeus that is unfortunately the correct behavior, the old behavior should have preserved the CEST timezone instead of correctly converting it to CET, unless that has gone awry.

@frondeus
Copy link

frondeus commented Dec 3, 2019

Hmm so I should probably operate on dates:

#[cfg(test)]
mod tests {
    extern crate chrono_tz;

    use self::chrono_tz::Europe::Warsaw;

    #[test]
    fn test_chrono_add() { // https://github.com/chronotope/chrono/issues/339
        use std::ops::Add;
        use chrono::*;

        let timezone = Warsaw;
        let mut date = timezone.from_utc_datetime(&"2017-10-28T22:00:00Z".parse::<DateTime<Utc>>()
            .unwrap()
            .naive_utc())
            .date(); //Note `.date()` at the end.

        dbg!(&date);
        assert_eq!("2017-10-29", &date.format("%Y-%m-%d").to_string());
        date = date.add(Duration::days(1));
        assert_eq!("2017-10-30", &date.format("%Y-%m-%d").to_string());
        date = date.add(Duration::days(1));
        assert_eq!("2017-10-31", &date.format("%Y-%m-%d").to_string());
    }
}

This works on both versions.

@quodlibetor
Copy link
Contributor

Interesting. That does make sense, although adding a duration to a date doesn't make sense! I'm glad you've found a workaround.

@quodlibetor
Copy link
Contributor

You could also convert to a NaiveDateTime and add a duration to that, which will ignore all timezone issues.

@oeed
Copy link

oeed commented Sep 10, 2020

I've run in to this too. The JavaScript data library Luxon does a really nice job of this by using 'time math' and 'calendar math'.

For examples like this, it's currently adding using time math (just 1 days worth of seconds), whereas sometimes you want to use calendar math (adding 1 day, regardless of how long that day is).

There's an explanation here: https://moment.github.io/luxon/docs/manual/math.html

@t-cadet
Copy link

t-cadet commented Feb 21, 2022

I have a use case for this too: I generate schedules for running tasks, and DST changes shift all subsequent datetimes, which is (often) undesirable

@esheppa esheppa mentioned this issue Aug 17, 2022
@pitdicker
Copy link
Collaborator

Fixed in #784.

@djc djc closed this as completed Jun 7, 2023
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 a pull request may close this issue.

7 participants