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

DateTime::years_since is tricky during ambiguous local time #1398

Open
pitdicker opened this issue Jan 31, 2024 · 5 comments
Open

DateTime::years_since is tricky during ambiguous local time #1398

pitdicker opened this issue Jan 31, 2024 · 5 comments

Comments

@pitdicker
Copy link
Collaborator

pitdicker commented Jan 31, 2024

Suppose the clock is turned back on say 2024-03-31 from 3:00 to 2:00 due to DST. We want to compare the current time with 2023-03-31 at 2:30:

let now = Local::now();
let base = NaiveDate::from_ymd_opt(2023, 3, 31).unwrap().and_hms_opt(2, 30, 0).unwrap();
let years_since = now.years_since(base);

The current behavior is that on 2024-03-31 it will return 0 years until 2:30. From the first 2:30 until 3:00 it will return 1 year as result. From the second 2:00 until 2:30 it will return 0 years. And after that 1 year again.

I am not sure what the 'right' result should be, but going back and forth seems not to be it.

@pitdicker
Copy link
Collaborator Author

pitdicker commented Jan 31, 2024

We can get a sensible result if we create an intermediate value with the date of now and the time of base, picking the earliest datetime if ambiguous. Then we compare the times of the intermediate value and now. (This only has to happen if the month and day are the same.)

@djc
Copy link
Contributor

djc commented Feb 1, 2024

I have questions:

  • Why does this behavior happen? Because a DST transition happens on that day, and we pick one of the sides?
  • Do we use years_since() in other calculations inside chrono?
  • If so, how, and what would the correct expected results be for those?

@pitdicker
Copy link
Collaborator Author

I was just reading the implementation of DateTime::years_since and wondering how it dealt with differences in timezones. Does it work in local time or UTC? and if in local time, how does it behave if the two dates have different timezones?

Turns out it works in local time (sensible). The method signature requires the timezone to be the same. That only leaves the case of the offset from UTC changing within the same timezone as a potentially tricky case.

  • Why does this behavior happen? Because a DST transition happens on that day, and we pick one of the sides?

Because of a DST transition on that day, and we don't pick any side but naively use the local time.

  • Do we use years_since() in other calculations inside chrono?

No. It is used nowhere, we don't even seem to have tests. Actually I was in the process of deprecating it but couldn't come up with a usable alternative.

  • If so, how, and what would the correct expected results be for those?

I suppose from the moment month, day and time are equal it counts as a full year that has passed. If the clock turns backwards after that it should keep counting that full year.

Should we care about this issue?
I just consider this a minor bug that is fun to fix (I also know more normal ways to have fun 😄). Maybe even a good first issue with some mentoring.
More importantly the solution informs how to do correct calculations with a CalendarDuration on DateTimes.

@djc
Copy link
Contributor

djc commented Feb 7, 2024

It's probably worth fixing but also not very high priority? Could do something like comparing the months and if the difference is larger than 1 return, same for months and days, then convert both to Utc and compare that?

@pitdicker
Copy link
Collaborator Author

I can probably write the fix described in #1398 (comment) and a test in two hours.

The reason to open this issue is that I should get in the habit of making notes such as this public instead of keeping a single line somewhere on my PC with 'TODO'. It seemed low priority enough for me to not work on directly and discuss in a PR 🤣.

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

No branches or pull requests

2 participants