Custom clippy configuration #478
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #478 +/- ##
=======================================
Coverage 58.02% 58.02%
=======================================
Files 216 216
Lines 15942 15942
=======================================
Hits 9250 9250
Misses 6692 6692
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
rydrman
approved these changes
Sep 5, 2022
jrray
commented
Feb 25, 2023
| @@ -34,6 +37,7 @@ fn make_tag_part(pieces: Vec<&str>) -> Option<Vec<BuildKeyVersionNumberPiece>> { | |||
|
|
|||
| // For making test case results | |||
| #[allow(clippy::too_many_arguments)] | |||
| #[allow(clippy::fn_params_excessive_bools)] | |||
There was a problem hiding this comment.
Allowing this here as the one special case in the code base.
jrray
commented
Feb 25, 2023
| #![deny(unsafe_op_in_unsafe_fn)] | ||
| #![warn(clippy::fn_params_excessive_bools)] | ||
|
|
||
| use std::sync::Arc; |
There was a problem hiding this comment.
Fixed a misplaced import here.
|
@rydrman please do another look before I merge this. I added comments to the changes that aren't just adding deny/warn to all the files. |
rydrman
reviewed
Feb 25, 2023
|
I'm not sure, I put it everywhere.
…On Fri, Feb 24, 2023, 10:16 PM Ryan Bottriell ***@***.***> wrote:
***@***.**** commented on this pull request.
I thought that we could put the lint attributes at the crate level and not
need to apply them to every module, or am I misreading these diffs?
—
Reply to this email directly, view it on GitHub
<#478 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAA3LMBSM5LTTCK32THWR3WZGPSLANCNFSM57VCAVMA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Do you know of a way to return a list of files in the project that are "at
crate level"?
…On Fri, Feb 24, 2023, 10:22 PM J Robert Ray ***@***.***> wrote:
I'm not sure, I put it everywhere.
On Fri, Feb 24, 2023, 10:16 PM Ryan Bottriell ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
>
> I thought that we could put the lint attributes at the crate level and
> not need to apply them to every module, or am I misreading these diffs?
>
> —
> Reply to this email directly, view it on GitHub
> <#478 (review)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAA3LMBSM5LTTCK32THWR3WZGPSLANCNFSM57VCAVMA>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
|
I think it would just be every |
|
Anything listed as a target in any Cargo.toml, build scripts, etc.
…On Sat, Feb 25, 2023, 1:23 PM Ryan Bottriell ***@***.***> wrote:
I think it would just be every lib.rs file
—
Reply to this email directly, view it on GitHub
<#478 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAA3LKULZI7O2XNG4KTC3LWZJZ4LANCNFSM57VCAVMA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Enable checking of excessive bool parameters in all crates, and limit to 1. Limit the number of arguments to 5. Signed-off-by: J Robert Ray <[email protected]>
|
I've taken the directives out of most files. The nice thing about having it in every file is that it is straightforward to verify that the directives are present and active in every file. |
rydrman
approved these changes
Mar 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a step towards having some coding and style conventions for this project "in writing" that we can all discuss and agree on, focusing on two things have been a frequent point of contention in code reviews.
The use of bools in function parameters can be monitored by clippy. This is a "pedantic" check so it is not enabled by default. I put the attr in every lib.rs to enable it. Our codebase passes with this set to 1, with one exception, where the warning has been supressed. I agree with the sentiment of the clippy lint text, "calls to such functions [with excessive use of bools] are confusing and error prone," particularly if there are multiple consecutive bool arguments. But rather than banning bool arguments entirely, allowing a single bool argument avoids the confusing argument order problem.
The threshold for too many arguments is enabled by default, but has a default limit of 7. For our codebase, 5 is the lowest it can go without triggering any warnings. My personal preference is to not try to go lower than 5.
Interestingly, this was the #2 most ignored lint: rust-lang/rust-clippy#7666 As noted in the comments, this is most likely due to
newconstructors for complex structs requiring many arguments.