Clippy Book Chapter Updates Reborn: Defining Lints #10595
Conversation
rustbot
added
the
S-waiting-on-review
|
I consider documentation very important and a team-wide effort, could you please read it and give your opinions (On a review or a one-off comment)? |
|
Weird cc @rust-lang/clippy |
blyxyas
force-pushed
the
book-define_lints
branch
from
9c74aff
to
3f3fc95
Compare
|
☔ The latest upstream changes (presumably #10605) made this pull request unmergeable. Please resolve the merge conflicts. |
blyxyas
force-pushed
the
book-define_lints
branch
from
3f3fc95
to
a9afbcf
Compare
There was a problem hiding this comment.
I'm not completely clear about the planned extent of this chapter, so I'm not sure if there's anything missing, but what I see looks good.
Perhaps adding some copy from the created files to show what should be changed (notably the lint declaration itself, e.g. the version annotation often trips up newbies) might help, but also take up some space, and I'm not sure if you want to tackle that in a "documenting lints" chapter.
| is an `early` lint pass. We will discuss this difference in a later chapter. | ||
| <!-- FIXME: Link that "later chapter" when lint_passes.md is merged --> | ||
| 2. If not provided, the `category` of this new lint will default to `nursery`. | ||
| See Clippy's [lint types](../lints.md) for more information on categories. |
There was a problem hiding this comment.
Here we have lint types linking to the upper chapter, and elsewhere there's a Lint Types heading, which makes me think the term might be overloaded. If so, could we find a more apt term here?
There was a problem hiding this comment.
Maybe changing that to link to the later section "Lint types", and adding a note in "Lint types" about how there's more information in ../lints.md could clear the confusion?
There was a problem hiding this comment.
I think that the latest commit would solve the issue.
Clippy Book Chapter Updates Reborn: Lint Passes This PR adds a new chapter to the book: "Lint passes". No major changes apart from some re-phrasing, fixing typos... etc. ## Notes - Requires #10595 to be merged before this one (Or else, a link will be broken). - To talk about the whole project, please use the tracking issue for the project #10597 (It also contains a timeline, discussions and more information) changelog: Add a new "Lint passes" chapter to the book r? `@flip1995`
There was a problem hiding this comment.
I would like this documentation get way slimmer. This still seems like a lot of stuff is copied over from adding_lints.md. This should be a short explanation on how to create lints. The example for foo_functions should be almost completely removed. I would either replace it with some generic lint_name in the example commands or keep the name, but then don't talk about what it does at all. This should go into a full-tutorial documentation, like what adding_lints is today.
I would also move the Lint Types up.
I'm unsure if Lint Name is placed well in this chapter. I'm inclined to completely remove it or slim it down to just one sentence that points to lint naming guidelines.
And finally, as llogiq pointed out, I would at least copy the define_clippy_lints macro that is generated into this documentation and explain each part and how it should be updated (documentation + version) before submitting the lint.
Basically what I'd like this document to become is a quick overview/collection of commands that could be run to generate new lints plus explanation what is generated, if it has to be generated manually.
|
☔ The latest upstream changes (presumably #10653) made this pull request unmergeable. Please resolve the merge conflicts. |
blyxyas
force-pushed
the
book-define_lints
branch
from
69035c3
to
d84039f
Compare
|
The latest commits should fix all issues |
There was a problem hiding this comment.
Only formatting changes left to do. I will address them myself and then merge this.
Amazing work. I will use this PR to apologize again, for not reviewing for so long. But I took a week of vacation from $day_job and can now give those PR the attention and time they deserve.
| These types group together lints that share some common behaviors. | ||
| For instance, `functions` groups together lints | ||
| that deal with some aspects of function calls in Rust. |
There was a problem hiding this comment.
Not blocking: We might want to add a one-line description for each type in the list above.
Question: Are function lints really about function calls, rather than function signatures/declarations? 🤔
There was a problem hiding this comment.
Yes it is mostly about declarations/signatures. I changed the wording a bit here.
flip1995
force-pushed
the
book-define_lints
branch
from
16b5722
to
783c119
Compare
|
@bors r+ I also rebased the branch on top of latest master. Let's get those PRs in |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Clippy Book Chapter Updates Reborn: Method Checking This PR adds a new chapter to the book: "Method Checking". Some major re-phrasing was done and some changes in the code comments (apart from grammar and minor changes). ## Notes - Requires #10595 **and** #10622 to be merged before this, or else several links will be broken - To talk about the whole project, please use the tracking issue for the project #10597 (It also contains a timeline, discussions and more information) changelog: Add a new "Method Checking" chapter to the book. r? `@flip1995`
Revival of #9659, I've made a few changes (mainly ones from reviews from #9426, like removing mid-section storytelling) and some re-writes. The goal of the project would be to slowly re-write the Clippy book (only chapters that need it) to more up-to-date style.
Notes
git statuscommands have changed, we need to make sure that they're correct.changelog: Add a new "Defining lints" chapter to the book
r? @flip1995