Skip to content

Experiment: Move lint docs into markdown files #6992

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

Closed
wants to merge 1 commit into from

Conversation

camsteffen
Copy link
Contributor

@camsteffen camsteffen commented Mar 28, 2021

changelog: none

This is an experiment to put the docs for each lint into a separate markdown file.

So each lint doc would be at clippy_lints/docs/$category/$name.md instead of inside declare_clippy_lint! { .. }.

I got inspired to do this when I saw that the feature for #[doc = include_str!(..)] has an open PR for stabilization (rust-lang/rust#83366). I was quite surprised how easy it was to implement. And there is virtually no added boilerplate.

Open questions/issues:

  • Do we want this?
  • It is slightly annoying that that file names are uppercase. This could be solved with some procedural macro to_lowercase!(name).
  • I propose we use markdown headings like ## What it does\n.. instead of **What it does:** ...
  • Need to fix cargo dev
  • Lint doc location scheme. Should there be subfolders for categories?
  • Will CI be okay?
  • Is env!("CARGO_MANIFEST_DIR") the best approach?
  • Should we merge all the declare_clippy_lint!s into one jumbo declare_clippy_lints!?
  • Do we still need the ALL the Clippy Lints page?

@rust-highfive
Copy link

r? @phansch

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 28, 2021
@camsteffen camsteffen changed the title Experiment: Move lint docs into separate files Experiment: Move lint docs into markdown files Mar 28, 2021
@ghost
Copy link

ghost commented Mar 28, 2021

This might not be the place for this comment, but I think Clippy should try moving away from storing the lint metadata in doc comments completely.

It's stored in it doc comments, then parsed out, then written back and used in a bunch kinds of places. It's convoluted. Why not just store the meta data directly in code as a collection of structs? Then the parsing can be skipped completely and the generating code is replaced with for loops acting on the structs.

The only thing that might be an issue is keeping doctests working and I think the doc-comment crate could help there.

@camsteffen
Copy link
Contributor Author

It's stored in it doc comments, then parsed out, then written back and used in a bunch kinds of places.

What are you referring to? We have a python script that extracts the docs from declare_clippy_lint!. But the contents of the docs are not parsed AFAIK.

@flip1995
Copy link
Member

flip1995 commented Mar 28, 2021

The only thing that might be an issue is keeping doctests working and I think the doc-comment crate could help there.

Yeah, we definitely want the doctests working.

It's stored in it doc comments, then parsed out, then written back and used in a bunch kinds of places

It's extracted by a python script into a json file and then this json file is directly used to generate the website. There aren't any other places where this information is used.


One concern I have here is that we'll have 450+ files for all our lints. Also defining the lint documentation in an extra file could be quite a PITA. When I look at a lint and want to fix a bug, I often look at the documentation first, which I can go with "Go to definition" in my editor. This wouldn't be possible.

Also moving a lint to a different category would be harder. But I guess update_lints could do this.

I don't really see a reason why we wouldn't want the documentation in our code. Especially with #6887 getting the documentation should be way cleaner and not rely on some python regex.

Do we still need the ALL the Clippy Lints page?

Ultimately we would want a Clippy book with all the lints. Looking something like this: https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html

Do we want this?

What would be the advantage of having those in an extra file?

@camsteffen
Copy link
Contributor Author

When I look at a lint and want to fix a bug, I often look at the documentation first, which I can go with "Go to definition" in my editor. This wouldn't be possible.

That's a good point. You could find the docs by searching for the file by name. That is something I am used to doing quickly but that's just me.

Especially with #6887 getting the documentation should be way cleaner and not rely on some python regex.

I tested the change with #6887. It just works. Doc tests also work.

What would be the advantage of having those in an extra file?

Shorter files to scroll through. Docs are not edited as frequently as implementation. Viewing and editing markdown files can be a better experience in various tools/IDE's, including GitHub. I would probably use the rendered view to read new docs when reviewing PR's.

quite a PITA

Lol. I totally see where you're coming from. More files is a con.

I think the lint docs are best seen as Clippy user documentation rather than library docs for const rustc_lint::LintId items. User documentation is typically separate from code. But this is just a philosophical note.

@flip1995
Copy link
Member

flip1995 commented Mar 29, 2021

Hmm, some good points. I personally still wouldn't do it, but now see why we would want to do it*. Let's hear some more opinions about this @rust-lang/clippy. We could make a final decision in Tuesday's meeting.

*EDIT: mostly because it breaks my own workflow and I think it could be annoying when writing new lints or working on existing lints.

@Manishearth
Copy link
Member

Yeah I do kinda find it super valuable that the lint docs are in the source next to the lint. I wouldn't block this change, but I have a preference towards the status quo.

@camsteffen
Copy link
Contributor Author

I'm inclined to close this then. I don't feel very strongly about it.

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.

5 participants