Add a pre-commit hook mirroring CI's fmt and clippy checks - #1372
ErwanLegrand wants to merge 1 commit into
Conversation
ErwanLegrand
changed the title
LastExceed
left a comment
LastExceed
left a comment
There was a problem hiding this comment.
I like this idea! There are a few things I'd change though:
| # Pre-commit hook: runs the same fmt and clippy checks as CI's Code Quality workflow | ||
| # (`cargo fmt --all -- --check` and `cargo clippy --all -- -D warnings`), adapted to the | ||
| # local platform. CI additionally lints each supported target with per-platform feature | ||
| # sets (e.g. `--all-features` with the ASIO SDK on Windows), which a local hook cannot. |
There was a problem hiding this comment.
I'd remove this paragraph, as it seems a bit redundant. It does not matter here what CI does, and the hook itself seems simple enough to not require summary.
| # CI skips documentation-only changes (paths-ignore: "**.md", "docs/**", "LICENSE*", ".gitignore"). | ||
| if ! git diff --cached --name-only --diff-filter=ACMR | grep -Eq '\.rs$|Cargo\.(toml|lock)$'; then | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
This comment implies a black-list, but the filter actually uses a white-list, so listing all the excluded files is redunant (and prone to be incomplete).
I also think that the filter is redundant in general, since there is no harm in running fmt and clippy on an unchanged code base, and who knows what kind of files not fitting the filter build.rs might depend on in the future.
| CI's Code Quality workflow runs `cargo fmt --all -- --check` and `cargo clippy --all -- -D warnings` | ||
| (for each supported target/feature set). A pre-commit hook running the same commands on the local | ||
| platform is provided: install it once with |
There was a problem hiding this comment.
Same as before, I'd remove the CI part.
Also, perhaps emphasize that this is just an optional tool, not a requirement. Keeping all commits 100% compliant is not always feasible, or even desirable, and having to explicitly use --no-verify might constitute unnecessary friction for some people, especially those who prefer to do cleanup at the end of each branch instead of each commit (which is a common workflow that IMO is perfectly fine).
eb8f7c8 to
148cc61
Compare
|
I have made the changes as requested. Let me know if you find anything else. |
2 participants
It's all in the title. The aim is to increase the quality of submissions before CI even runs, obviously.