Add support for running crate specific extra_lints.sh
#21
Conversation
There is are some tabs and inconsistent indentation. Replace all tabs with spaces and make the indentation consistent.
no-default-features is not applied by clippy on a workspace. Remove it and the if statement that skips corepc which does not compile with no-default-features.
|
I checked this by running the CI lint using this branch on |
Some lints need to be run at the crate level, e.g. no-default-features. Other crate specific lints can be useful in CI. Add support for running crate specific extra_lints.sh scripts.
jamillambert
force-pushed
the
0610-extra-lints
branch
from
5eebceb to
856e83d
Compare
| build_and_test | ||
| ;; | ||
|
|
||
| lint) |
There was a problem hiding this comment.
Was just poking around this repo for the first time and appreciate the whitespace clean up
| need_nightly | ||
|
|
||
| $cargo clippy --workspace --all-targets --all-features --keep-going -- -D warnings | ||
| $cargo clippy --workspace --all-targets --keep-going -- -D warnings |
There was a problem hiding this comment.
I'll admit I am not sure on the driving reason for this change, so my bad if a dumb question, but what's the reasoning on dropping --all-features from the default flag set? My gut says it's better to check all the code by default, and then have the crate-specific stuff search for weird feature combos.
There was a problem hiding this comment.
Sorry I forgot to link to the issue in rust-bitcoin that started this all. I have updated the description to mention it now.
The bit that was dropped is --workspace --no-default-features, I am not sure exactly what clippy does, but it does not lint the crates with no-default-features when you use it with workspace.
Dropping --all-features for a second run of clippy was always there, it was just, incorectly, under an if statement that skipped corepc for no-default-features.
There was a problem hiding this comment.
Ah ok, thanks for stepping through that for me. So now is the second clippy run testing the default features while the first is all?
There was a problem hiding this comment.
Yes, we also thought we were doing a third run with no features. But as you can see in the related issue this is not the case. EDIT: I say "we" but really this is my first contribution to this repo 😊.
There was a problem hiding this comment.
Yea makes sense. Is there any benefits to running the default features one? Feels a little murky what that would actually resolve to for the workspace.
There was a problem hiding this comment.
I think so, only because when I was testing this change there was a lint error in https://github.com/rust-bitcoin/corepc that only comes up with the second clippy with default features. It didn't come up before because that line was not run for corepc.
| for crate in $CRATES; do | ||
| pushd "$REPO_DIR/$crate" > /dev/null | ||
| if [ -e ./contrib/extra_lints.sh ]; then | ||
| ./contrib/extra_lints.sh |
There was a problem hiding this comment.
Would be nice to add this new interface to the README. Might just be my naive/fresh eyes, but I found the README really helpful to understand the interface of this repo.
There was a problem hiding this comment.
Thanks, I agree. I just forgot.
Added a line to the README.
Add documentation for `extra_lints.sh` script.
|
utACK 7c6233a |
|
Nice, works as advertised. |
|
Excuse the delay, I was not watching this repo :( You can always poke me if things sit for 2 days because something probably went wrong since I usually check notifications everyday (excl. Sunday). |
4 participants
Some lints need to be run at the crate level, e.g. no-default-features dos not run correctly with clippy on a workspace. Other crate specific lints can also be useful in CI.
This was all motivated by the issue rust-bitcoin/rust-bitcoin#5060