-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(manual_assert_eq): new lint #16025
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: master
Are you sure you want to change the base?
Conversation
|
r? @Alexendoo rustbot has assigned @Alexendoo. Use |
be6ca8c to
4db706b
Compare
|
Lintcheck changes for ada768b
This comment will be updated if you push new changes |
5856d0e to
7ae07b6
Compare
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.
The suggestion is only valid if the types being compared implement Debug.
7ae07b6 to
6b87ed6
Compare
|
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
6b87ed6 to
3e38dfd
Compare
This comment has been minimized.
This comment has been minimized.
3e38dfd to
154a53b
Compare
| #[expect( | ||
| clippy::manual_assert_eq, | ||
| reason = "the message contains `assert_eq!`-like formatting itself" | ||
| )] |
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.
We shouldn't lint if there is an assert message provided
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 agree that the suggestion is unfortunate in this case, but in general, a provided assert message can often complement the default one from assert_eq
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.
Maybe reduce the applicability in this case?
tests/ui/assertions_on_constants.rs
Outdated
| #[allow(clippy::manual_assert_eq, reason = "tests `assert!` specifically")] | ||
| { | ||
| assert!(8 == (7 + 1)); | ||
| //~^ assertions_on_constants | ||
| } |
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.
The allows in this and other unrelated tests should go in the top level #![allow] like other lints, they don't need a reason
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.
In my experience, putting such specific allows (and not things like no_effect, which one really wants to apply to the whole file) on the specific locations that trigger them helps keep the test suite clean, as it's easier to notice when a particular allow becomes unnecessary (this has happened to me more than once). Specifying a reason is also helpful for that.
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.
Moving to expects is something we might want to consider to know when we need to cleanup, though that comes with its own problems
But I think having irrelevant lints inline is a distraction personally
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.
Fine fine.. moved the allows to the top, switching to expect wherever possible
This comment has been minimized.
This comment has been minimized.
154a53b to
925a38c
Compare
This comment has been minimized.
This comment has been minimized.
925a38c to
4d400a2
Compare
This comment has been minimized.
This comment has been minimized.
4d400a2 to
903359a
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
903359a to
ada768b
Compare
Resolves #13252
Resurrection of #13333
changelog: [
manual_assert_eq]: new lint