-
Notifications
You must be signed in to change notification settings - Fork 92
Add comprehensive linting #1327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1327 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 75 75
Lines 12764 12757 -7
=========================================
- Hits 12764 12757 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# | ||
# Note we still need rustfmt for the cxx-qt-gen tests | ||
- name: "Install Rust toolchain" | ||
run: | | ||
rustup toolchain add 1.78.0 --component clippy | ||
rustup toolchain add 1.87.0 --component clippy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before we were testing in CI with our MSRV, do we not want to do this now?
Or instead should something like clippy be in it's own runner using a newer Rust toolchain 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, clippy could also just be separate runner, as I think it doesn't share many build artifacts with the main build anyhow.
@@ -65,8 +65,26 @@ serde_json = "1.0" | |||
thiserror = "1.0" | |||
|
|||
[workspace.lints.clippy] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess my question would be why are these lints not enabled by default? Do we want to enable so many? I usually follow the defaults for lints as there is sometimes a reason why they are not enabled yet. As otherwise this can become an explosion of debates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No clippy lints are enabled by default as far as I'm aware. The nice thing about making lints opt-out instead of opt-in is that there are a lot of lints and I think most of them are useful—I learned some new best practices in the course of making this PR, such as clippy::ptr_as_ptr. This PR has actually been something I've been working on for a while because it has been a useful exercise for me, and it has yielded other PRs in the process such as #1324 and #1321. So the effort was worthwhile in itself, even if this PR doesn't get merged.
Enables most Clippy lints.
Most of these changes were automatically applied using
cargo clippy --fix
. Nearly all of them are simple syntactic adjustments like adding a semicolon at the end of a line. The remaining changes are things like removing unused&self
parameters and accepting&str
instead ofString
if ownership isn't necessary.I can break those up into multiple PRs if that's preferred, but I figured I'd start by putting them all in one place so people can take a look and decide what they think.