Enforce no clippy warnings through CI #2148
Conversation
This allows us to run the exact config from CI locally.
thomaseizinger
force-pushed
the
enforce-no-clippy-warnings
branch
from
91d0492 to
5bfc35e
Compare
There was a problem hiding this comment.
Thanks for tackling this @thomaseizinger. One small comment, other than that, this looks good to me.
.github/workflows/ci.yml
Outdated
| @@ -152,8 +152,7 @@ jobs: | |||
| - name: Run cargo clippy | |||
| uses: actions-rs/cargo@v1.0.3 | |||
| with: | |||
| command: clippy | |||
| args: -- -A clippy::type_complexity -A clippy::pedantic -A clippy::style | |||
| command: lint # cargo alias to allow reuse of config locally | |||
There was a problem hiding this comment.
Mhh, do I understand correctly that the alias would be needed to ease local development, in other words in order not to have to type clippy -- -A clippy::type_complexity -A clippy::pedantic -A clippy::style -D warnings before commiting? Though not particularly important, I don't think the additional convenience is worth the indirection, especially since I don't expect contributors to run into many clippy lints.
There was a problem hiding this comment.
since I don't expect contributors to run into many clippy lints.
The issue is that without specifying -A clippy::type_complexity -A clippy::pedantic -A clippy::style clippy returns something like 200 results for rust-libp2p :)
In case of a CI failure, users need to specify this entire configuration to reproduce the errors locally. Once clippy ships its configuration file, I think we should switch to that but until then, I see this alias as the best option we have to make fixing linting errors locally reasonably smooth.
There was a problem hiding this comment.
👍 sounds good to me. Thanks for the additional reasoning.
I would just suggest renaming lint to something like custom-clippy or something along those lines.
Opened as a result of #2139.
Might also want to change the GitHub configuration to pervent merge if clippy fails.