-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
chore: multipart suggestions for let_unit_value lint #13754
base: master
Are you sure you want to change the base?
chore: multipart suggestions for let_unit_value lint #13754
Conversation
Flip1995 has the context of these, so if they have time: r? flip1995 (But this is still a draft!) |
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.
Great work! Just 2 codestyle comments.
Please also rebase your PR on top of the current master. We changed our CI recently and GitHub isn't good at picking this up on older PRs.
36b6b3f
to
36e6a98
Compare
Hey @flip1995 thanks for the review! I rebased and addressed the resulting dogfooding lints and your own comments, so there's a bit of a delta on top of what you reviewed. Should be good to go, though, I reckon. I am assuming you squash onto master, but if not, I can clean my commit history up in future 🤣 |
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.
Thanks! We don't squash on merge. So, yes, please clean up the commit history :)
} | ||
|
||
// Emit appropriate diagnostic based on whether there are usages of the let binding | ||
match suggestions.len().cmp(&1) { |
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.
That's a bit of overkill 😅 I would suggest to change this to:
if !suggestions.is_empty() {
let message = if suggestions.len() == 1 {
"omit the `let` binding"
} else {
"omit the `let` binding and replace variable usages with `()`"
};
diag.multipart_suggestion(...);
}
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.
Yep, that's way less awful. Let me clean it up 👍
db9276a
to
a29525a
Compare
a29525a
to
ec24388
Compare
@flip1995 that should be it! 🙏 |
Hey @flip1995 , do you have a chance to have a quick look at this? We're very close to finishing the whole issue! |
Sorry for not replying. I won't get to review any Clippy PRs until around christmas... I'll reassign r? clippy |
This should address #13099 for the let_unit test.
changelog: [let_unit]: Updated let_unit to use multipart_suggestions where appropriate