-
Notifications
You must be signed in to change notification settings - Fork 55
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
Applied clippy lints #1734
Applied clippy lints #1734
Conversation
Ignore that lint for now.
Yes, we've enabled the squash merge option precisely for that reason. |
Yes, this has to be done in a specific PR. See #1716 |
Robot Results
Passed Tests
|
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 would approve these changed with the exception of the as u64
fix. The later makes sense but we need to double check as this has already led to an issue #592.
We avoid squash merge. Here I would make an interactive rebase to separate the boring fixes from those subtle enough to have deserved some comments. |
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.
Approved. But, I'm still wondering why these clippy warnings were never emitted earlier, even with the clippy checks that are part of our CI.
Okay, so the reason these lints were not caught either on CI or on local dev environments, is because rust toolchain version is pinned to 1.64 on CI and on dev environments (via But at the same time, we use So on the one hand, we check lints with But clippy knows about the MSRV and some lints are gated behind MSRV so that lints that take advantage of newer features are not shown when below MSRV specified in the manifest. So there is no reason to use So I propose the following:
I first stumbled upon this when revising MSRV for Yocto, and thought that it's not relevant, so I didn't mention it, yet it came up almost immediately. So I think now I'll be reporting this kind of stuff immediately as it comes up. @didier-wenzek @PradeepKiruvale Let me know what you think. |
Not fully correct. We are using nightly *only for
I'm okay with that. This is indeed a bit confusing when trying to update locally for a more recent toolchain.
Agree to change nothing here.
Okay on each developer's fork/clone but why also on the CI?
I would encourage the developers to use |
It's so that we can use new features in tests without them failing on the CI, because tests are only run during development and not by the end users, who only care about building the binary. I admit that there's not much room to utilise new features in unit and integration tests, but in theory there's nothing that would prohibit as from doing so, so we might as well do it - was my thinking. |
I am also ok with using |
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.
Approved
@@ -222,7 +223,7 @@ fn create_file_and_try_pre_allocate_space( | |||
file.as_raw_fd(), | |||
FallocateFlags::empty(), | |||
0, | |||
file_len as nix::libc::off_t, | |||
file_len.try_into().expect("file too large to fit in 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.
expect
will panic right? @didier-wenzek Is it ok to have that 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.
In practice it will never panic, because for the u64 -> i64
conversion to fail, the file would have to be bigger than 2^63-1
, which comes up to roughly 8 exabytes. Maybe it is a meaningless change, but file_len
is a function parameter, and if somebody manages to call it so horribly wrong, this would panic whereas as
would've silently proceeded, by either truncating or overflowing, not sure which. Not that it matters because the called function would also probably return an error, but in principle, using .into()/.try_into()
is preferred to as
, so why not. Will probably look into converting as
to using into
in the rest of the codebase as well some other time.
@@ -174,7 +172,8 @@ mod tests { | |||
|
|||
#[test] | |||
fn control_chars_are_removed() { | |||
let input = generate_test_vec_u8(); | |||
let input: Vec<u8> = (0x00..0xff).collect(); | |||
dbg!(&input); |
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.
remove dbg!
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.
Oh my bad, was testing if it will really be the same and forgot to remove it.
"{\"type\":\"ThinEdgeMeasurement\",\"temperature0\":{\"temperature0\":{\"value\":0.0}}" | ||
)); | ||
assert!(result[0].payload_str().unwrap().contains( | ||
"{\"type\":\"ThinEdgeMeasurement\",\"temperature0\":{\"temperature0\":{\"value\":0.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.
why not raw string 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.
Forgot to convert, will fix
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.
LGTM, left a few comments.
Signed-off-by: Marcel Guzik <[email protected]>
Signed-off-by: Marcel Guzik <[email protected]>
Signed-off-by: Marcel Guzik <[email protected]>
Signed-off-by: Marcel Guzik <[email protected]>
Signed-off-by: Marcel Guzik <[email protected]>
Signed-off-by: Marcel Guzik <[email protected]>
This test is flaky:
Created an issue: #1741 |
Will fix. One more thing: clippy has even more lints groups that are allowed by default. I tried changing You can run |
I would not bother too much with pedantic warnings. Indeed, numerous but not really useful as "consider using an underscore-prefixed named binding" instead of |
Never mind, I did not notice this issue was assigned to you @PradeepKiruvale, I'll leave the fix to you then. |
Proposed changes
Using rust 1.69, there were quite a lot of lints reported by
cargo clippy
. Most of them are applied in this commit, however one particular type of lint remains and there were some ambiguities.Types of changes
Paste Link to the issue
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments
Most of the lints were very boring things, like removing unnecessary borrows.
One type of lint I did not correct, which is
the Err-variant returned from this function is very large
lint which lints large error types returned inResult
s because result is as large as its largest variant, and in some cases I've seen function that haveOk(())
variant withErr
variants being in excess of 200 bytes, which might not be optimal, but fixing it would involve boxing stuff and changing behaviour, so this should be looked at in the separate PR.Finally, there was one ambiguity i encountered:
thin-edge.io/crates/tests/mqtt_tests/src/test_mqtt_client.rs
Lines 210 to 214 in 9fb001d
This was changed to await the future as I figured this must've been the intent.
Finally, when merging the PR, can we use squash merge for the PR? I've divided it into commits for review purposes, but it can be merged as 1 commit, and Github can do that one itself.