-
Notifications
You must be signed in to change notification settings - Fork 715
feat: expand logic to determine local time #10509
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
base: master
Are you sure you want to change the base?
Conversation
|
Mathlib CI status (docs):
|
|
Reference manual CI status:
|
|
Thanks for the PR!
You can add a new test directory to |
b14c138 to
1355a6b
Compare
This PR extends TZdb to honor the `TZ` environment variable and fall back to `UTC` if `/etc/localtime` does not exist. This is more consistent with C and allows overwriting local time as unprivileged user.
1355a6b to
705bd71
Compare
|
Thanks! I think the test works now |
|
Is there anything blocking this MR? |
| return result | ||
|
|
||
| if let some path := env then | ||
| for path in default.zonesPaths do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you're changing the logic by using the default paths instead of the paths given by the database that the user is supplying. This is not correct, we want users to configure their own timezone database location from application code if they are so inclined.
| if ← System.FilePath.pathExists path then | ||
| let result ← readRulesFromDisk path id | ||
| return result | ||
| throw <| IO.userError s!"cannot find {id} in the local timezone database" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some subtleties around the paths here that I think we should clear up. The docstring of the zonesPaths field reads
All the possible paths to the directories containing all available time zone files. These files define various time zones and their rules.
To me this reads like getZoneRules should search for the given time zone in all of the supplied paths and return the rules if it finds them in any of the paths.
This is not what currently happens (neither before nor after the PR). The readRulesFromDisk function throws an exception if it does not find the zone rules for the given ID, so the code here only looks for the given time zone in the first directory in zonesPaths that exists. So if we hit this throw clause here, we are actually in the situation that we didn't find any time zone database.
@algebraic-dev, what did you intend the behavior to be here? I see two options:
- Keep the current behavior where we only search the first path that exists. In that case we should clarify the docstring of
zonesPathsand change the error message here to something likedid not find a local timezone database in <list of paths>. - Change the behavior so that we search all paths and only report an error if we didn't find the rules in any of the paths. This would mean refactoring
readRulesFromDiskandgetZoneRules(and clarifying the docstring ofzonesPathsmight still be a good idea).
Just a lack of review capacity :) |
This PR extends TZdb to honor the
TZenvironment variable and fall back toUTCif/etc/localtimedoes not exist.This is more consistent with C and allows overwriting local time as unprivileged user.
I would kindly ask for help with writing a test for the
TZenvironment variable since I am very new to lean. More specifically: How should I set an environment variable for a test?