Add a set of standard clippy lints #10612
Comments
Kanabenki
added
C-Feature
alice-i-cecile
added
A-Build-System
|
For an initial PR, I'd like to do only those CI warnings we currently use. Then, add one lint at a time in their own PRs. That will ensure each can be evaluated on its own, and reduce time-to-merge. |
|
It could maybe be worth it to also add a |
|
I think that's probably more complexity in the workflow than it's worth. Because this is a FOSS project with a huge range of contributors, we try to keep our project config and workflows simple where we can :) |
alice-i-cecile
added
the
D-Trivial
# Objective Partially Addresses #10612 fix: add clippy::doc_markdown linting to cargo workspace Rather than do all the warnings in `tools/ci/src/main.rs` in one-shot, just wanted to have an initial pr adding the first one to get the form correct as some may trigger build errors and require changes to get merged more easily. ## Solution - adding the doc_markdown and removing it from the ci check as it'll now be a build error during normal compilation. --------- Co-authored-by: François <mockersf@gmail.com>
Partially addresses bevyengine#10612
# Objective Partially addresses #10612 After moving the initial docs_markdown warning, this is a PR that moves the rest of the CI clippy lint definitions to the cargo.toml. ## Solution - the `tools/ci/src/main.rs` clippy lints removed and just the warning flag remains. - the warnings moved to the root cargo workspace toml.
# Objective Related to #10612. Enable the [`clippy::manual_let_else`](https://rust-lang.github.io/rust-clippy/master/#manual_let_else) lint as a warning. The `let else` form seems more idiomatic to me than a `match`/`if else` that either match a pattern or diverge, and from the clippy doc, the lint doesn't seem to have any possible false positive. ## Solution Add the lint as warning in `Cargo.toml`, refactor places where the lint triggers.
…#10640) # Objective Partially Addresses bevyengine#10612 fix: add clippy::doc_markdown linting to cargo workspace Rather than do all the warnings in `tools/ci/src/main.rs` in one-shot, just wanted to have an initial pr adding the first one to get the form correct as some may trigger build errors and require changes to get merged more easily. ## Solution - adding the doc_markdown and removing it from the ci check as it'll now be a build error during normal compilation. --------- Co-authored-by: François <mockersf@gmail.com>
|
What are the remaining items that aren't addressed on this issue? |
|
Going to close as completed: new lints should have their own issues and PRs :) |
What problem does this solve or what need does it fill?
There's currently no project-level clippy lints defined.
What solution would you like?
Once #10011 is merged, we could expand the
Cargo.tomllint table with a set of clippy lints we want to uphold through the codebase. Something we could use as inspiration is the set of clippy used in Embark projects. While some lints in it are probably too restrictive/opinionated, we could filter it and possibly check for additional lints that might fit the codebase in the full lint list.We could then remove the few clippy warnings currently checked in CI, since they would be picked up from the lint table. This could be done over the course of several PRs since the resulting diff might be large.
What alternative(s) have you considered?
Cargo.toml.The text was updated successfully, but these errors were encountered: