-
Notifications
You must be signed in to change notification settings - Fork 277
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
Use offset when getting time duration #1307
Conversation
598e678
to
2a3707e
Compare
cc @Byron just confirming that |
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.
Thanks for reeling me in.
Generally I recommend adding way more tests, and also see what happens with unusual times which are absolutely possible. Future dates and pre-epoch dates, which I believe definitely shouldn't panic.
Ok(s) => s, | ||
Err(_) => return "<before UNIX epoch>".into(), | ||
Err(_) => return Err("<before UNIX epoch>".into()), |
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.
gitoxide
supports times before unix epoch, but of course, there are dates outside of the valid range of i64
.
/// Gets the duration between `now` and `time`. Returns `Err` if this cannot be calculated. | ||
fn diff_gix_time(now: SystemTime, time: Time) -> Result<Duration, String> { | ||
let since_epoch_duration = now.duration_since(SystemTime::UNIX_EPOCH).unwrap(); | ||
let ts = Duration::from_secs(match (time.seconds + i64::from(time.offset)).try_into() { |
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'm a bit confused. Why do we need to consider the offset here? This method should simply return the difference between two UTC timestamps (without timezone): now (since epoch in UTC+0) - commit.time (since epoch in UTC+0
.
Sorry if I'm mistaken.
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 made this with the assumption that #1306 was caused by differing timezones, which was an incorrect assumption, which made me assume that we were comparing a timezone-aware time to a naive time. Right now, personally, I am not sure if commit.time.seconds
is UTC+0 or if it's a naive time that needs offset
to get the real time.
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.
Sorry, I also didn't catch it. commit.time.seconds
is UTC. The doc-string there unfortunately only implies it ("time in seconds since epoch."). I have fixed this here.
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.
Thanks! I guess this PR should be closed, then.
Related: #1306