Run Clippy on CI #2201
Run Clippy on CI #2201
Conversation
| run: | | ||
| # Install Clippy (not included in the `minimal` profile) | ||
| rustup component add clippy | ||
| cargo clippy --all --all-targets |
There was a problem hiding this comment.
I would add -- -Dwarnings here -- let me know if that's desired
There was a problem hiding this comment.
Unclear if we want to block triagebot CI on clippy. Any opinions @rust-lang/triagebot?
There was a problem hiding this comment.
I think I would not like to enforce Clippy in the CI. I gave my option already in #1950, unsure why this work about Clippy linting moved forward.
There was a problem hiding this comment.
I personally use cargo clippy -- -Dwarnings on many Rust projects, but no strong opinion on having it here. If you two are opposed, I'm fine with not enforcing it.
There was a problem hiding this comment.
I would not like to enforce Clippy in the CI
If I may ask, what are you concerns with enforcing clippy lints?
unsure why this work about Clippy linting moved forward.
Because clippy lints are generally useful, and I did exactly what you suggested in the issue1: I carefully reviewed the changes, asked for them to split up, vetoed some lints, all of that to improve the quality of the codebase.
Footnotes
There was a problem hiding this comment.
what are you concerns with enforcing clippy lints?
clippy is a bit of a moving target so linting this codebase moves the requirements for contributors as well. My understanding is that clippy is meant more as a "personal choice" for the developer. We prevent contributors to choose their own style of linting. If there are disagreements on a lint we then would need to add allow/deny directives which (imo) is noise in the code.
For all these reasons I think I don't see many Rust projects enforcing linting besides rustfmt but I'd like to hear if there are counterarguments.
There was a problem hiding this comment.
clippy is a bit of a moving target
Not having -- -Dwarnings helps with that a bit, as on a new Clippy version, the CI will in the worst case get a couple more warnings, instead of breaking completely.
On the other hand, my personal argument for -- -Dwarnings is that it much more strongly stops new PRs from introducing additional warnings, requiring every PR to "clean-up" after itself.
We prevent contributors to choose their own style of linting
At least the warn-by-default lints are meant to be fairly uncontroversial -- if there's disagreement about enabling any of those, that could be considered a reason to ask for the lint to be downgraded to allow-by-default.
There was a problem hiding this comment.
Personally I'm not a fan of the default clippy set of warnings. The correctness and complexity groups are the only ones that seem relatively safe to enable. perf, style, and suspicious tend to have too many false positives or noise for my taste (or I just disagree with them). There are individual lints that are useful, but the list is so huge that figuring that out is too much of an annoyance for me.
However, I'm not terribly active here, so I don't have a strong say.
5 participants
Follow-up to #2181