Skip to content
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

Add lint writing documentation #3824

Merged
merged 18 commits into from Mar 9, 2019
Merged

Add lint writing documentation #3824

merged 18 commits into from Mar 9, 2019

Conversation

phansch
Copy link
Member

@phansch phansch commented Feb 26, 2019

Rendered

This adds a new documentation page that explains how to write Clippy
lints. It guides the reader through creating a foo function lint.

I plan to iterate a bit more on the prose of some sections, but I think the
general structure is fine now, so I'm looking forward to feedback =)

One thing I'm not sure about: I felt like this is too big for CONTRIBUTING.md
so I put it into a new doc/ directory. I can imagine having more
documentation in the future, so we might even want to create a book using
mdbook instead? Or should everything go into CONTRIBUTING.md?

Further things left to do:

  • Link from CONTRIBUTING.md
  • Remove things covered in this guide from CONTRIBUTING.md
  • Section about clippy::author attribute
  • Run remark-lint on CI over the doc directory and fix things

@Manishearth
Copy link
Member

This looks really good so far, thanks for doing this!!

As an aside: We should probably build a longer version of the lint naming conventions, we have a bunch of our own (like useless_foo). https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints is pretty basic

Copy link
Member

@detrumi detrumi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff! A few comments after reading through

doc/adding_lints.md Outdated Show resolved Hide resolved
doc/adding_lints.md Outdated Show resolved Hide resolved
doc/adding_lints.md Outdated Show resolved Hide resolved
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First off, thanks for the great work, this document will be really useful!

Maybe also mention the #[clippy::author] attribute.

In the future I would like a section "Advanced Lints", where some other useful things are mentioned and explained. I'm thinking of:

  • Visitor
  • LateLintPass example
  • PreExpansionPass
  • Constants
  • MIR-Lints
  • ???

Or we can add additional documentation files for this, to keep this one short (which is probably the better idea)

doc/adding_lints.md Outdated Show resolved Hide resolved
doc/adding_lints.md Outdated Show resolved Hide resolved
doc/adding_lints.md Show resolved Hide resolved
@phansch
Copy link
Member Author

phansch commented Feb 28, 2019

Thanks for the reviews so far! I added a small TODO list to the PR description. I will fix those and give it another read/try later today.

In the future I would like a section "Advanced Lints", where some other useful things are mentioned and explained. I'm thinking of: Visitor, LateLintPass example, PreExpansionPass, Constants, MIR-Lints

These are all good ideas! I hope having the documentation in it's own directory lowers the bar a tiny bit for new documentation. I found that working with the existing CONTRIBUTING.md was getting a bit unpleasant.

@phansch
Copy link
Member Author

phansch commented Mar 1, 2019

I didn't have time to re-read the whole thing yet and try it out, but I think I addressed all the remaining issues, including the cleanup of CONTRIBUTING.md.

@phansch phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Mar 1, 2019
doc/adding_lints.md Show resolved Hide resolved
doc/adding_lints.md Outdated Show resolved Hide resolved
@felix91gr
Copy link
Contributor

Whaaat! This PR is awesome 😍 there's so much info I learned this last week as I got started, that you've neatly put together here. Thanks! This will surely help a lot of people 😊

@felix91gr
Copy link
Contributor

Have you seen this? Maybe it'd be nice to connect it with this PR :D

@phansch phansch changed the title [WIP] Add lint writing documentation Add lint writing documentation Mar 4, 2019
@phansch phansch added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 4, 2019
@phansch
Copy link
Member Author

phansch commented Mar 4, 2019

Have you seen this? Maybe it'd be nice to connect it with this PR :D

Yup, I just cleaned that up a bit and added a 'Done' column and created #3843.

Apart from that, I think this PR is ready for a final review. The rest can be done in subsequent PRs.

doc/adding_lints.md Outdated Show resolved Hide resolved
@phansch phansch added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 6, 2019
@phansch phansch added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 8, 2019
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just skimmed over the new document and I vote for merging this.

@phansch
Copy link
Member Author

phansch commented Mar 9, 2019

Going to go ahead and get this merged then, we can still improve it going forward ✨

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 9, 2019

📌 Commit 34685a5 has been approved by phansch

@bors
Copy link
Collaborator

bors commented Mar 9, 2019

⌛ Testing commit 34685a5 with merge 920112d...

bors added a commit that referenced this pull request Mar 9, 2019
Add lint writing documentation

[Rendered](https://github.com/phansch/rust-clippy/blob/adding_lints/doc/adding_lints.md)

This adds a new documentation page that explains how to write Clippy
lints. It guides the reader through creating a `foo` function lint.

I plan to iterate a bit more on the prose of some sections, but I think the
general structure is fine now, so I'm looking forward to feedback =)

One thing I'm not sure about: I felt like this is too big for CONTRIBUTING.md
so I put it into a new `doc/` directory. I can imagine having more
documentation in the future, so we might even want to create a book using
mdbook instead? Or should everything go into CONTRIBUTING.md?

Further things left to do:

- [x] Link from CONTRIBUTING.md
- [x] Remove things covered in this guide from CONTRIBUTING.md
- [x] Section about `clippy::author` attribute
- [x] Run `remark-lint` on CI over the `doc` directory and fix things
@bors
Copy link
Collaborator

bors commented Mar 9, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: phansch
Pushing 920112d to master...

@bors bors merged commit 34685a5 into rust-lang:master Mar 9, 2019
@phansch phansch deleted the adding_lints branch March 9, 2019 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants