.cargo: Run clippy on ALL the source files#2949
Conversation
Previously, we were running clippy without --workspace which left out examples and tests of all sub-crates.
thomaseizinger
force-pushed
the
clippy-workspace
branch
from
05a8bb5 to
04a01e7
Compare
thomaseizinger
force-pushed
the
clippy-workspace
branch
from
04a01e7 to
8ceba76
Compare
thomaseizinger
left a comment
thomaseizinger
left a comment
There was a problem hiding this comment.
Made some mistakes apparently when fixing the lints.
thomaseizinger
force-pushed
the
clippy-workspace
branch
from
8ceba76 to
cb4da08
Compare
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
First off, thanks for doing this!
Thanks for making it easy to review.
Thanks for making it very entertaining!
Goodbye to all those .clone() calls.
|
|
||
| fn prop(num_nodes: u8, seed: u64) -> TestResult { | ||
| if num_nodes < 2 || num_nodes > 50 { | ||
| if !(2..=50).contains(&num_nodes) { |
There was a problem hiding this comment.
Not that it is important, though I am curious, do folks find this easier to read?
There was a problem hiding this comment.
Pretty sure this one was done by clippy, I don't remember writing that.
If I were to write this test, I would probably split it into two ifs:
if num_nodes < 2 {
return TestResult::discard();
}
if num_nodes > 50 {
return TestResult::discard();
}There was a problem hiding this comment.
Or make a newtype that implements Arbitrary that only generates numbers between 3 and 49. We can call it NonZeroOneTwoU6Minus15!
There was a problem hiding this comment.
👍 on the above suggestions in general, though I think in this particular case leaving it as is is just fine.
| let inactive = servers.split_off(1); | ||
| // Drop the handles of the inactive servers to kill them. | ||
|
|
||
| #[allow(clippy::needless_collect)] // Drop the handles of the inactive servers to kill them. |
There was a problem hiding this comment.
out of curiosity to understand in this case why clippy was complaining I commented the allow and ran the cargo +nightly clippy --all-features --all-targets -- -A clippy::type_complexity -A clippy::pedantic -D warnings command. But clippy didn't complain, is it not required?
There was a problem hiding this comment.
You need the --workspace flag! My best guess is that this is because our root Cargo.toml is a crate and a workspace declaration.
I wonder whether we could avoid this if we put the meta crate in a directory too.
Thoughts @mxinden?
There was a problem hiding this comment.
yeah, was running outdated rust version 😅 sorry. Btw with the latest nightly there are new lints triggered for large_enum_variant.
There was a problem hiding this comment.
yeah, was running outdated rust version 😅 sorry. Btw with the latest nightly there are new lints triggered for
large_enum_variant.
Yeah I know :)
I opened #2951 for this recently!
Currently, our top-level `Cargo.toml` manifest represents a crate AND a workspace. This causes surprising behaviour (e.g. #2949) where we need to explicitly pass `--workpace` to every command to run it on the entire workspace and not just the meta crate. My moving the meta crate into its own directory, the root manifest file is a virtual manifest only and thus, every `cargo` command will automatically default to running on the entire workspace. On top of this, I personally find it easier to understand if workspace and crate manifests are not mixed. Pull-Request: #3536.
3 participants
Description
I turns out you need a PhD in clippology to figure out how to have it lint every source file in a repository. Previously, we were not passing
--workspaceto clippy which means it didn't lint a lot of source files. I am not sure which subset it was linting and which one it skipped but with this PR, we should be linting every source file in this repository.Review guide
This PR is best reviewed patch-by-patch. The massive diff comes from the gossipsub tests but most of that is just removing one layer of modules which is done in a separate commit.
clippy --fix. Probably not worth reviewing. If you run the command mentioned in the commit locally and follow it withcargo fmt, you should get the same result!testsmodule one layer up, fixing the last clippy lint. I tried to split this up so the diff shows how minimal the changes are but Git chokes on it and likes to see unchanged newlines instead of just showing the whitespace diff. You can verify locally that it is just whitespace changes by running:git diff 9443c7a4 9443c7a4^ --ignore-all-spacecargo fmt. You can again verify this locally if you want. Splitting this out makes the previous diff command work.Links to any relevant issues
Open Questions
Change checklist
I have made corresponding changes to the documentationI have added tests that prove my fix is effective or that my feature worksA changelog entry has been made in the appropriate crates