Add a "more granular lint groups" page to the book #13501
Conversation
rustbot
added
the
S-waiting-on-review
book/src/granular_lint_groups.md
Outdated
| @@ -0,0 +1,219 @@ | |||
| # More granular lint groups | |||
|
|
|||
| Clippy groups its lints into [9 primary categories](lints.md), | |||
There was a problem hiding this comment.
I just noticed that lints.md doesn't have the nursery category documented, so that page actually only describes 8
y21
changed the title
tests/lint-groups.rs
Outdated
| let regex = Regex::new( | ||
| "(?s)\ | ||
| (?<header><!-- lint-group-start: (?<name_start>[\\w-]+) -->)\ | ||
| (?<lints>.*?)\ | ||
| (?<footer><!-- lint-group-end: (?<name_end>[\\w-]+) -->)\ | ||
| ", | ||
| ) |
There was a problem hiding this comment.
In clippy_dev::update_lints there is a function called replace_region_in_{file,text} that does the same thing as this, but without regex.
Why not implement this in clippy_dev and use the facilities that are already there. It is probably a good extension for the update_lints subcommand.
There was a problem hiding this comment.
Regex was slightly more convenient here because we also need to capture part of the start pattern, which replace_region_in_text didn't really allow and now needs small amounts of manual parsing, but I think it's fine.
This also required a bunch of changes to replace_region_in_text:
- The text in between the start/end pattern in
replace_region_in_textis now passed to the replacement function since we need it to get the subgroup name - It now does multiple replacements if needed (necessary here because there are multiple
<!-- lint-subgroup -->sections)
tests/lint-groups.rs
Outdated
| use itertools::Itertools; | ||
| use regex::{Captures, Regex}; | ||
|
|
||
| const GROUPS: &[(&str, &[&str])] = &[ |
There was a problem hiding this comment.
Did you think about adding an optional "subgroup tag" to the define_clippy_lints macro and then use that information to generate those groups, instead of listing them out here?
This might be better in the long run. WDYT?
There was a problem hiding this comment.
Yeah, that sounds like a better idea. Implemented that for the macro, though not sure if the current syntax is a good idea.
I was initially worried that the term "lint group" was too ambiguous for this because we already use "lint group" and "lint category" interchangeably everywhere which already has its own meaning, but you mentioned "subgroup" which I like more as a name for this, so it uses that consistently now.
y21
force-pushed
the
lint-groups
branch
3 times, most recently
from
fcdb36a
to
00175a7
Compare
|
☔ The latest upstream changes (presumably #13639) made this pull request unmergeable. Please resolve the merge conflicts. |
This PR adds a new page to the book for more granular, "light" lint groups that are more informal than the real groups and can be easily added by anyone since it's "just" a page in the book.
I originally proposed this on Zulip a while ago: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/.22Which.20lints.20should.20I.20enable.22.20page and people seemed to generally like the idea.
For the implementation, there are two parts involved:
The book page is partly generated and partly written by hand. Everything in between lint-group-start and lint-group-end is generated/checked by a test, everything else is manually written. There is also a comment on the page that explains this.
Having this extra test is probably not technically necessary and we could also just have a single
#[warn(..)]list for every group, but this has the advantage that we can also provide the lint table code and do some extra checking, e.g. to make sure that all lints actually exist (preventing typos or lint names going stale if one gets renamed, ...).The actual code for generating it is less than 100 lines (excluding the data), so it doesn't seem like much extra complexity.
Another check that we could add (but doesn't exist right now) is that all listed lints actually live in that category (so that e.g. nursery-perf lints are removed when they get moved out of nursery)
changelog: none