-
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
Implement internal lint for MSRV lints #8313
Conversation
/// ### What it does | ||
/// Check that the `extract_msrv_attr!` macro is used, when a lint has a MSRV. | ||
/// | ||
pub INVALID_MSRV_ATTR_IMPL, |
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.
Nice to see an implementation this quickly after the suggestion! I only skimmed over the code, maybe MISSING_MSRV_ATTR_IMPL
would be a slightly better name 🤔. 🙃
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.
@xFrednet feel free to self-assign this PR if you like
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'm still trying to limit my work on Clippy while writing my bachelor thesis. Clippy is just more fun and productive 😂. I have some other reviews waiting on me, if those are further along, I can take another look at it. But that might take some time, so for a timely review, it's probably better if you stay assigned. But thank you very much for the offer! :)
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.
Yeah I was unsure about the name.
(I intentionally assigned Cam, so that you don't get distracted again)
@camsteffen Since you collected a lot of reviews recently, you can leave this PR at the back of your review queue. There shouldn't be any rush getting this in.
☔ The latest upstream changes (presumably #8451) made this pull request unmergeable. Please resolve the merge conflicts. |
b87cfd5
to
bcee65a
Compare
Done 👍 |
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 implementation looks good to me. I found two small NITs and there is the open suggestion to rename the lint. The current name is also fine with me if you prefer this one 🙃.
I hope I wasn't to nit picky ^^
This internal lint checks if the `extract_msrv_attrs!` macro is used if a lint has a MSRV. If not, it suggests to add this attribute to the lint pass implementation.
bcee65a
to
bca4ee7
Compare
Addressed all comments, renamed lint, and rebased 👍 |
@rustbot ready |
Looks good to me, thank you for the new lint. I also had fun reviewing it 👍 @bors r+ |
📌 Commit bca4ee7 has been approved by |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
This internal lint checks if the
extract_msrv_attrs!
macro is used ifa lint has a MSRV. If not, it suggests to add this attribute to the lint
pass implementation.
Following up #8280 (comment). This currently doesn't implement the documentation check. But since this is just an extension of this lint, I think this is a good MVP of this lint.
r? @camsteffen
cc @xFrednet
changelog: none