-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Verify that versions in the changelog match the lint definitions #14821
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
This comment has been minimized.
This comment has been minimized.
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.
LGTM overall. Some open questions
@@ -1,3 +1,5 @@ | |||
#![expect(clippy::invisible_characters, clippy::non_ascii_literal)] |
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.
Why is this now necessary 🤔
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 file!()
string literal has a strange span (e.g. clippy_lints/src/unicode.rs:56:1: 73:2 (#0)
), it points to the whole declare_clippy_lint! { ... }
callsite. The lints pick up that snippet and see the characters in the doc comment
Before it was part of a concat
rather than its own expression
} | ||
|
||
assert!( | ||
checked > 400, |
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.
Where is this magic number coming from? 🤔
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.
It was the number at the time rounded down to the nearest 100, just a basic check that if the format of the entire changelog ever changed or it became a stub linking to a site or something like that then the test would fail
let mut in_new_lints = true; | ||
let mut checked = 0; | ||
|
||
for event in Parser::new(&changelog) { |
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.
This parsing seems a bit brittle. It relies on the changelog format never changing. Do you have ideas how we can make this more robust? For example, add a cargo dev
command that inserts changelog headings automatically.
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'll add a note next to the template in the docs, it should be visible in the review diff if the New Lints
part changes at least
244d7ea
to
5e3c7a4
Compare
A complement to #14299 (could be merged in either order)
It checks the changelog
## New Lints
sections for lints and compares against the#[clippy::version]
in their definitionEssentially just automates https://doc.rust-lang.org/clippy/development/infrastructure/changelog_update.html#4-update-clippyversion-attributes, doesn't require any process changes
r? @flip1995
changelog: none