Skip to content

Explore time zone lookup errors - #1454

Closed
pitdicker wants to merge 3 commits into
chronotope:0.5.xfrom
pitdicker:tz_lookup_error
Closed

Explore time zone lookup errors#1454
pitdicker wants to merge 3 commits into
chronotope:0.5.xfrom
pitdicker:tz_lookup_error

Conversation

@pitdicker

@pitdicker pitdicker commented Feb 22, 2024

Copy link
Copy Markdown
Collaborator

To have something to talk about for #1448.

The Windows code is pretty clean and has OsError and InvalidTimeZoneData as error causes.

The Unix code remains in dire need of a refactoring.
I've tried to hook it up reasonably well to the error causes InvalidTzString, TimeZoneUnknown, TimeZoneNotFound, and InvalidTimeZoneData. NoTzdb isn't hooked up yet.

@pitdicker

Copy link
Copy Markdown
Collaborator Author

The Unix code remains in dire need of a refactoring.

I did a rewrite in #1457. As it turns out the caching logic was pretty much broken beyond the most basic cases.

What I have come to see more clearly related to this PR: we have three fallbacks. Any error is just swallowed and the next fallback is tried. The last fallback is to use the UTC time zone.

That we try multiple methods to determine the time zone is only logical.
However if we determine the time zone, but its configuration turns out to be broken, I am not sure it is good to continue trying other fallbacks.
Silently falling back to UTC while the user expects a local time doesn't seem great to me. We are not limited to fitting everything in an infallible API no matter what, like libc. We already return Options or LocalResults.

I'll open an issue to discuss for 0.5.

@pitdicker pitdicker mentioned this pull request Feb 24, 2024
@djc

djc commented Feb 27, 2024

Copy link
Copy Markdown
Member

Silently falling back to UTC while the user expects a local time doesn't seem great to me. We are not limited to fitting everything in an infallible API no matter what, like libc. We already return Options or LocalResults.

Agreed. IIRC we tried this in the beginning but it was too big of a backwards compatibility issue. Would like to address this in 0.5.

@pitdicker pitdicker closed this Mar 11, 2024
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