-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Minor: Fix clippy
by switching to timestamp_nanos_opt
instead of (deprecated) timestamp_nanos
#7572
Conversation
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 think timestamp_nanos()
was deprecated as it can panic internally -- timestamp_nanos_opt
can not panic (and returns None if the value can not be represented)
clippy
by switching to timestamp_nanos_opt
instead of (deprecated) timestamp_nanos
.unwrap_or(0), | ||
Self::EndTimestamp(timestamp) => timestamp | ||
.value() | ||
.map(|ts| ts.timestamp_nanos() as usize) | ||
.and_then(|ts| ts.timestamp_nanos_opt()) |
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 this correct, it will convert a panic into 0?
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 don't expect that the metrics would produce timestamps that can't be converted to nanoseconds so it is somehwhat a theoretical difference.
I could see the argument being made to preserve the existing behavior (panic
if somehow the values can't be represented as a timestamp) but I also think returning 0 rather than panic'ing in that case (what this PR does) is also reasonable
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.
Just one niggle
in order to get CI back healthy, I am going to merge this PR in. If anyone has other opinions about the |
I think this needs a follow on to pin chrono: #7575 |
Which issue does this PR close?
Closes #.
Rationale for this change
https://crates.io/crates/chrono/0.4.31 was released recently and it appears to deprecated
timestamp_nanos
. This results in local clippy failures for meWhat changes are included in this PR?
Use the non-deprecated API
Are these changes tested?
by existing tests
Are there any user-facing changes?