Replace time crate with jiff #15293
Replace time crate with jiff #15293
Conversation
rustbot
added
A-configuration
|
r? @epage |
| @@ -619,7 +620,7 @@ fn auth_token_optional( | |||
| if let Some(cached_token) = cache.get(url) { | |||
| if cached_token | |||
| .expiration | |||
| .map(|exp| OffsetDateTime::now_utc() + Duration::minutes(1) < exp) | |||
| .map(|exp| Timestamp::now() + Duration::from_secs(60) < exp) | |||
There was a problem hiding this comment.
Small thing, but you could do Timestamp::now() + SignedDuration::from_mins(1) here.
There was a problem hiding this comment.
Is there some benefit to use SignedDuration instead of std's Duration? Latter is just shorter to write and does the same thing :)
There was a problem hiding this comment.
You get the from_mins constructor. But otherwise, in this case, no.
| @@ -367,7 +367,6 @@ dependencies = [ | |||
| "tar", | |||
| "tempfile", | |||
| "thiserror 2.0.11", | |||
| "time", | |||
There was a problem hiding this comment.
Unfortunately it looks like this PR doesn't actually remove time from the dependency tree. As far as I can tell, it's still being brought in by pasetors.
It looks like it's primarily used by Cargo, so maybe they'd be open to a switch as well.
There was a problem hiding this comment.
Thanks for highlighting this.
To be clear, this wouldn't be a blocker for this PR
| #[serde(with = "time::serde::timestamp")] | ||
| expiration: OffsetDateTime, | ||
| #[serde(with = "jiff::fmt::serde::timestamp::second::required")] | ||
| expiration: Timestamp, |
There was a problem hiding this comment.
Isn't this a public API change and needs a major version bump?
Wonder why cargo-semver-checks didn't report to us.
There was a problem hiding this comment.
If so, should we wait for jiff 1.0?
There was a problem hiding this comment.
Or change it to other integer type or wrapper type, so it is completely opaque.
There is no guarantee that jiff@2 won't come out a week after jiff@1 is out.
There was a problem hiding this comment.
I agree an integer or wrapper type makes sense for something like this.
There is no guarantee that jiff@2 won't come out a week after jiff@1 is out.
Just to comment on this, it is my intent that jiff will stay at 1.0 indefinitely. For a jiff 2.0 to happen a week or even 1 month after jiff 1.0 is released, I think something spectacular would have had to go wrong.
Otherwise, I do have a track record of sticking to 1.0. regex is still at 1.0 even though there are mistakes in its API I would like to fix. Same with bstr.
There was a problem hiding this comment.
Hey @BurntSushi! I totally agree with it and fully trust you as a maintainer of those awesome crates. I meant to express that regardless it is 0.2 or 1.0, a major version bump is a major version bump. If we have fear of re-export we should make it opaque.
Sorry I didn't mean to make you feel bad 😞.
There was a problem hiding this comment.
No I didn't feel bad, I just want to make sure the right kinds of expectations are telegraphed. :) I just mean it would be a spectacular failure on my part if 2.0 were released right after 1.0.
My plan is to release Jiff 1.0 this summer.
There was a problem hiding this comment.
Yes and we already had this problem with time in the puiblic API. I was just wondering if we should consolidate the breaking changes or going ahead and moving forward. No strong opinion. So long as the serialization format is unchanged, breaking changes to this API are not too big of a deal; the target audience is very small.
There was a problem hiding this comment.
Should the braking changes (whatever they are) be done in another PR before this? Or in this PR?
There was a problem hiding this comment.
If we are removing time in this PR, the breaking change seems better to be in the same PR. Could be one more commit if needed.
Slightly prefer the approach making it opaque. Not really strong though.
PR #15290 replaced
humantimecrate withjiffand this PR replacestimecrate in favor ofjiff.This should not have functional change.