-
Notifications
You must be signed in to change notification settings - Fork 21
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
Replace humantime dependency for jiff #382
base: main
Are you sure you want to change the base?
Conversation
crates/trycmd/src/runner.rs
Outdated
@@ -68,10 +69,11 @@ impl Runner { | |||
status.spawn.status.summary(), | |||
); | |||
if let Some(duration) = status.duration { | |||
let duration = SignedDuration::from_nanos(duration.as_nanos() as i64); |
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.
Is status.duration
a std::time::Duration
? If so, consider SignedDuration::try_from(duration).unwrap()
, which will have an explicit panic instead of a silent truncation.
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 was not sure if we assumed panic was allowed in this context, but I see now it's already used at various places
Just updated the PR with your code (and thanks for jiff 😄)
Think of unwrap
as an assert with such a clear intention that it doesn't need more clarification.
There are different ways of thinking of this
- If a test were to take longer than
i64
can represent, that is highly unusual and a panic isn't too bad. In that case, we should document that with an.expect()
- If a test takes longer than a
i64
can represent, we should still report results and it would be fine to leave the value ati64::MAX
- If a test takes longer than a
i64
can represent, we should still report results and we should indicate that the test too longer than we could track
Thank you for the swift review @BurntSushi . |
Pull Request Test Coverage Report for Build 13818241505Details
💛 - Coveralls |
94822fd
to
591db74
Compare
From what I see, the MSRV check is a bit puzzling snapbox/crates/trycmd/CHANGELOG.md Line 83 in fbcd538
snapbox/crates/trycmd/CHANGELOG.md Line 83 in fbcd538
I would upgrade the one in https://github.com/assert-rs/snapbox/blob/main/Cargo.toml#L9 to |
I unofficially lowered the MSRV as an experiment. It should be fine to bump it up to 1.70 though that should be its own commit. |
3b7df67
to
dcd31f3
Compare
Hello,
Just recently
humantime
was referenced as RUSTSEC. This PR replaces it withjiff
Solving https://github.com/rustsec/advisory-db/blob/a99f72f78f43ef5b7883126f5454d86d8670b97d/crates/humantime/RUSTSEC-2025-0014.md?plain=1#L4
Not sure if the parsing of this is covered by tests though