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

Allow guaranteed-no-panic conversion from NaiveDate to DateTime #927

Closed
wants to merge 1 commit into from

Conversation

Ekleog
Copy link
Contributor

@Ekleog Ekleog commented Dec 30, 2022

Hello people,

First, thank you for making chrono! It's very helpful to write code that (hopefully) will work on any timezone.

However, this "make it work on any timezone" makes me scared about doing some stuff that still sounds really reasonable.

So I'm opening this PR to kill two birds with one stone: asking my question ("how can I make a DateTime at the first second of a day"), and providing an easy way for others to know and use the answer.

Basically, I'm wondering whether the implementation of this PR is correct. Is it possible for eg. a summer time change change to happen across midnight?

If the implementation of this PR is always correct, then I think it should land, because otherwise writing the code as an end-user of the library does not make it obvious that it is actually panic-free. And this leads to convoluted ways to work around it. For instance my current solution is I already have a DateTime, and add 86400-its number of seconds since the beginning of the day; and that gives me a very-approximated-during-summer-time-changes but guaranteed no-panic solution.

If the implementation of this PR is incorrect, how would you go about solving the problem at hand, and do you think it would be possible to integrate it into chrono itself?

Either way, have a good end-of-year holiday!

@Ekleog Ekleog force-pushed the patch-2 branch 2 times, most recently from 1707cd2 to c4299a5 Compare December 30, 2022 04:15
@esheppa
Copy link
Collaborator

esheppa commented Jan 2, 2023

Hi @Ekleog - thanks for the PR.

Basically, I'm wondering whether the implementation of this PR is correct. Is it possible for eg. a summer time change change to happen across midnight?

Regarding this, I'm not sure whether this does occur in the existing tzdb however I don't think there is any restriction against it occurring, and so we have to assume essentially any possible transition could occur (potentially aside from cases like an entire day being skipped).

For instance my current solution is I already have a DateTime, and add 86400-its number of seconds since the beginning of the day; and that gives me a very-approximated-during-summer-time-changes but guaranteed no-panic solution.

If you already have a valid DateTime<Tz> at the start of your desired day, you can do:
(Please note, there is a bug with this which will be fixed in 0.4.24, in the meantime please use the 0.4.x branch if you are using this feature)

// if there is an intervening offset transition, 
// this will add the required number of hours (less or more than 24) 
// to ensure that the output has the same local timestamp as the LHS input
let next_start = my_date_time + Days::new(1);

If the implementation of this PR is incorrect, how would you go about solving the problem at hand, and do you think it would be possible to integrate it into chrono itself?

The PR looks like a good start, and will be correct in some cases, but unfortunately it may be incorrect in timezones which have offset transitions.

Firstly getting the start of day as a NaiveTime should be possible infallibly using the below as of e7d4402.

let my_naive_date_time = my_naive_date.and_time(NaiveTime::MIN);

However adding the timezone is problematic for some timezones as that specific local time may not exist in that timezone. Essentially this works for any Utc or FixedOffset but not timezones that have offset transitions.

What you can do is the following for UTC (this works because UTC has the same local and UTC time)

let my_utc_time = Utc.from_utc_datetime(my_naive_date_time); // variable from above

And the following for FixedOffset

let my_fixed_offset = FixedOffset::east_opt(7 * 60 * 60); 
let my_fixed_offset_time = my_fixed_offset.and_utc_datetime(my_naive_date_time - my_fixed_offset); // Assuming the naivedatetime is local, translate it to equivalent UTC time

Perhaps it would be useful to add equivalent MIN_UTC and MAX_UTC to NaiveDate given that Date<Tz> is now deprecated. Would you be interested in doing that?

Separately it would be interesting to investigate having a min_tz function which finds the earliest valid time on that date in that timezone. We have a basic implementation here but as you can see it is not ideal as it has to loop, potentially with some heuristics.

@Ekleog
Copy link
Contributor Author

Ekleog commented Jan 3, 2023

Awesome, thank you for your answer! I hadn't known of your #883 PR. With it I think my problem would be solved!

I think this PR can thus reasonably be closed, as I don't see anything else actionable here.

@Ekleog Ekleog closed this Jan 3, 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 this pull request may close these issues.

2 participants