-
Notifications
You must be signed in to change notification settings - Fork 69
chore: add clippy rules for codebase consistency #804
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
Conversation
KolbyML
left a comment
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.
Can you add a CI to ensure we don't forget to add lints.workspace = true in the future, we should be able to use this https://crates.io/crates/cargo-workspace-lints ?
|
Converting to draft as I'm adding more rules. |
uninlined_format_args)|
This is very nice. This doesn't have to be perfect imo. Once this change is in place, if it feels like over 80 out of 100 to us, that’s good; if it feels over 90, that’s awesome. |
KolbyML
left a comment
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.
looks good, we should use needless_pass_by_value
|
actually there is a few cases were it wouldn't make sense and I am not sure if the linter is able to pick that up or not so, never mind |
What was wrong?
Our PR reviews often contain this kind of comment, let Clippy handle it.
How was it fixed?
Rationale
let_and_return(Link): Few cases for assigning variables before returning.uninlined_format_args(Link): Inline arguments as possible as we can. Note that this rule couldn't detect macros fromtracing.unnecessary_debug_formatting(Link): Usepath.display()for printing the file path.unwrap_used(Link): Stop usingunwrap()for production code, useexpect()instead.useless_let_if_seq(Link): I think this style is better :)wildcard_imports(Link): This rule reminds me of this comment, which is reasonable.Side note
I guess
needless_pass_by_value(Link) can be helpful and is often a topic for our PR comments, but this leads so many LOC to satisfy the linter. I'd like the team to share opinions on it.To-Do