-
-
Notifications
You must be signed in to change notification settings - Fork 557
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
Add timezone configuration option & CLI overrides #1517
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Generally looks good, thank you!
If there are any other options, then I'd rather not. I'm not aware of any other crates, though
Tests would be good, even if very bare. It would be nice to ensure this continues to work with any possible future changes too Otherwise, would you be able to update the docs too please? |
So I tried to implement named timezones using It turns out (unsurprisingly so) that I'll add tests and call it a day I guess. If ever in the future we decide to migrate to using chrono, I will be happy to add this retroactively. |
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.
I wrote some basic unit tests. Maybe they'll help ensure that in the future a different implementation won't change the behaviour, but apart from that they really aren't too helpful.
atuin/src/command/client/history.rs
Outdated
fn can_parse_local_timezone_spec() -> Result<()> { | ||
let local_tz = Timezone(UtcOffset::current_local_offset()?); | ||
|
||
assert_eq!(Timezone::from_str("l")?, local_tz); | ||
assert_eq!(Timezone::from_str("local")?, local_tz); | ||
Ok(()) | ||
} |
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.
Also, this test is failing on Linux. I looked at it with a debugger, and noticed a contradiction between time
's code and documentation. I'll submit an issue there to ask about it.
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.
I did some digging, and it appears that there's no mistake in time
; rather it's my own misunderstanding. Still though, I have to find a way to fix this test. Has to do with some really annoying thread-safety issues so it might take me a while to get it right.
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.
So I am going down this massive rabbit hole that basically boils down to "there's no thread-safe way to get a system's timezone on POSIX systems". To be honest I'm completely flustered at this point.
To deal with this, time
has implemented some rather stringent checks to ensure safety. This means this test either has to run in a single-threaded environment, or we have to accept unsoundness (which admittedly can only ever happen if libc's setenv
function is called directly without locking, but still). I'll see what I can do tomorrow.
Hey! Just wondering if there's anything I can do to help this one move forwards? 🙏 |
Thanks for offering. Here's where I'm at: As mentioned previously, the safety checks implemented by Currently, the specific problem is that There's also the long-term concern that this is essentially a landmine if in the future we ever want to make |
I've also explored the very hacky method of obtaining the local timezone using a shell command, something like this: #[cfg(target_os = "linux")]
let offset = {
let tz_raw = process::Command::new("date").arg("+%:::z").output()?.stdout;
UtcOffset::parse(std::str::from_utf8(&tz_raw)?.trim(), OFFSET_FMT)?
};
#[cfg(not(target_os = "linux"))]
let offset = UtcOffset::current_local_offset()?; Unfortunately, not even this is viable because unlike GNU date, POSIX date only supports printing named timezones. I'm honestly dumbfounded that such a simple problem can cause so much trouble. |
I guess I'm just looking for ideas at this point. As a last resort we can get rid of local timezone support altogether, although it really is the last resort. |
I've also been unable to find anything that would be a reasonable workaround, and keep local timezone support in this way. What might work is
Doesn't quite achieve everything we'd like to, but is 80% of the way there I think. |
also, thank you for looking into this so much, ik it's been more than expected |
This pull request has been mentioned on Atuin Community. There might be relevant details there: |
This is already the case, which is why the feature works but the test fails. I can add some comments in relevant places to remind future maintainers of this requirement.
From my POV I don't really see a reason to make a global config for this. But maybe that's just because I'm tunnel-visioned on this single feature while you have a broader view of the whole project. Can you please clarify what other components of
This test already exists as |
I think if we keep the timezone-reading separate to everything else, we don't need to call it in the tests - if it's going to be this difficult, anyway.
I'm being asked about timezones a lot right now, essentially for any area that displays time. In the TUI, in the stats, etc. A global config means we can resolve that everywhere. |
This pull request has been mentioned on Atuin Community. There might be relevant details there: https://forum.atuin.sh/t/can-i-use-local-timezone-for-my-history/101/2 |
I'm working on adding timezone as an option in settings, but saw that there is already the field Currently it's marked with Edit: I thought I might as well just do it now and change it if necessary. Please take a look at the code, thanks. Edit 2: I only realised this after the change, but this unification is a breaking change. This is because previously |
Rebased. |
Ahhh I'm sorry I totally blanked on that 😭
Seems reasonable
I think for general guidance on timezones, we should follow
|
couple of minor comments, but looks great - thank you! |
Only a few conflicts left and I'll get it merged! |
`--tz` is kept as a visible alias
Rebased. |
I was convinced I'd merged this, sorry! |
Thank you! Really happy to get this in 🙏 |
When reviewing history (usually for the purpose of filling out a work log), I often find it helpful if the timestamps are displayed in the local timezone. This PR adds this feature.
Currently, the
--tz
option either accepts an offset from UTC in the form of<+|-><hour>[:<minute>[:<second>]]
, or the special valuel
/local
to use the system's current timezones.I also wanted to add support for named timezones using
chrono_tz
, but that seems to require pulling inchrono
as well, which feels very redundant considering that we already depend ontime
. So I left that bit out for now.Unresolved questions
Are you (maintainers) willing to addchrono
as a dependency for named timezone support? Or are there alternative, independent crates tochrono_tz
?