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

Adapting the lint list to Clippy's new metadata format #7279

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented May 26, 2021

This is close to the end of a long living project to rewrite the lint metadata collection for Clippy. Progress on this has been tracked in #7172. This PR adds one of the last missing puzzle pieces, the actual display of all the changes that have been done in the background. A preview can be seen here: Clippy's lint list

The styling has been discussed on zulip but is still open to suggestion.

Side note: It has been fun working on the website where we don't have unit tests and everything is just tested by playing around. However, it's good that this chaos is contained into this one part of Clippy. 🐛


Closes: #1303
Closes: #4310

This actually closes fewer things than I thought it would...

changelog: Reworked Clippy's website:
changelog: * Added applicability information about lints
changelog: * Added a link to jump to the specific lint implementation
changelog: * Adapted some styling and improved loading time

r? @flip1995

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 26, 2021
@xFrednet xFrednet marked this pull request as draft May 26, 2021 18:18
@xFrednet xFrednet force-pushed the 7172-adapt-website-to-new-format branch 2 times, most recently from 974580e to c0f8291 Compare May 26, 2021 18:30
util/gh-pages/index.html Outdated Show resolved Hide resolved
util/gh-pages/index.html Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member Author

xFrednet commented Jun 15, 2021

New to-dos:

@bors
Copy link
Contributor

bors commented Jun 15, 2021

☔ The latest upstream changes (presumably #7352) made this pull request unmergeable. Please resolve the merge conflicts.

@flip1995
Copy link
Member

I got another idea for the website (definitely not for this PR though): Add a link to playground for ```rust code blocks from the documentation. No idea how hard that would be. @xFrednet do you think this would somehow be possible?

@xFrednet
Copy link
Member Author

Hey, I think that should be possible to implement this in some form, rustdoc already has similar functionality. I believe the biggest hurdle would be to adapt all lint documentation to have valid code. Some examples are currently using just a code snipped that would be invalid on its own. (Examples: (1) (2) (3)). The code would also need to include a lint level for the lint like #[deny(lint_name)] to make sure that it gets triggered.

What is the main objective of this feature? @flip1995

I would only use it to view the output of such a lint. However, I think that that use case might be better covered by an implementation that includes the lint output in the documentation itself. Rustc does this by running the example code and replacing the string {{produces}} with the actual strerr output (from my understanding). Having the lint output in the documentation can make it a bit overloaded, but the output could easily be hidden inside a foldable section to create something like this:

// Bad
let _ = "Hello".bytes().nth(3);

// Good
let _ = "Hello".as_bytes().get(3);
Lint output
warning: called `.byte().nth()` on a `str`
 --> src/main.rs:4:9
  |
4 | let _ = "Hello".bytes().nth(3);
  |         ^^^^^^^^^^^^^^^^^^^^^^ help: try: `"Hello".as_bytes().get(3)`
  |
  = note: `#[warn(clippy::bytes_nth)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bytes_nth

(Sorry if the last part totally misses the point of your question. This was a thing that I thought about while working on a lint in rustc and my mind made the connection to your question)

@flip1995
Copy link
Member

All code in the documentation tagged with ```rust can be run, since we already check it with rustdoc tests. We include # ... lines for this:

/// ```rust
/// # let mut a = 1;
/// # let mut b = 2;
/// a = b;
/// b = a;
/// ```

Yeah my intention is pretty much to run Clippy on the code and see the output. I don't think this feature will be used very often, so it shouldn't be a priority.

This is more something that would be nice to have, if you or someone else has an idea on how to add this non-intrusively. If it is much work to add it, I really don't think it is worth adding at all.

@xFrednet
Copy link
Member Author

Okay, I'll add that as an idea to the tracking issue 🙃

Comment on lines 523 to 529
// This removes the leading space that the macro translation introduces
if let Some(stripped_doc) = line.strip_prefix(' ') {
docs.push_str(stripped_doc);
} else if !line.is_empty() {
docs.push_str(&line);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The rebase might make it difficult to see where and how the conflicts were resolved. I added this block to strip the leading space on every filled line and added a doc comment to the method itself, the rest stayed the same. I also removed the JS implementation of the language extraction in a new commit.

@xFrednet xFrednet force-pushed the 7172-adapt-website-to-new-format branch from e5f8927 to 3958620 Compare June 26, 2021 12:25
@xFrednet xFrednet marked this pull request as ready for review July 2, 2021 18:43
@xFrednet xFrednet mentioned this pull request Jul 28, 2021
1 task
@flip1995
Copy link
Member

@bors treeclosed=1000

@flip1995
Copy link
Member

I don't understand CSS or JS, so I can't really review those changes, but a quick skim over the changes and looking at the resulting website LGTM.

Approving to do a rollup.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 28, 2021

📌 Commit 08a9cb9 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jul 28, 2021

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Jul 28, 2021
…ormat, r=flip1995

Adapting the lint list to Clippy's new metadata format

This is close to the end of a long living project to rewrite the lint metadata collection for Clippy. Progress on this has been tracked in rust-lang#7172. This PR adds one of the last missing puzzle pieces, the actual display of all the changes that have been done in the background. A preview can be seen here: [Clippy's lint list](https://xfrednet.github.io/rust-clippy/master/index.html)

The styling has been discussed on [zulip](https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Styling.20feedback.20for.20Clippy's.20lint.20list/near/239601067) but is still open to suggestion.

Side note: It has been fun working on the website where we don't have unit tests and everything is just tested by playing around. However, it's good that this chaos is contained into this one part of Clippy. 🐛

---

Closes: rust-lang#1303
Closes: rust-lang#4310

This actually closes fewer things than I thought it would...

changelog: Reworked Clippy's [website](https://rust-lang.github.io/rust-clippy/master/index.html):
changelog: * Added applicability information about lints
changelog: * Added a link to jump to the specific lint implementation
changelog: * Adapted some styling and improved loading time

r? `@flip1995`
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Jul 28, 2021
…ednet,flip1995

Switch CI to new metadata collection

r? `@xFrednet`

Things we have to keep in mind:

- This removes the template files and the scripts used for deployment from the checkout. This was added in rust-lang#5517. I don't think we ever needed those there. Not sure though.
- ~~As a result, we can't remove the python scripts yet. We have to wait until this hits a stable Clippy release.~~ I'll just break the next stable deploy and do it by hand once.
- This should be merged together with rust-lang#7279. Me and `@xFrednet` will coordinate the switch
- ...?

I still have to try out some things:

- [x] Is it worth caching? Yes
- [x] ~~Is it worth to do a release build?~~ Nope
- [x] Does it actually work? With a few changes, yes
- [ ] ...?

changelog: Clippy now uses a lint to generate its documentation 🎉
@flip1995 flip1995 force-pushed the 7172-adapt-website-to-new-format branch from 08a9cb9 to dbddce1 Compare July 28, 2021 13:03
Changes included:
    - Minimum adaption to the new `lints.json` format
    - Fixing filtering for the new `lints.json` format; hardcoding the
      lint groups in the index
    - Recreating the original doc styling for the new format
    - Fixed sytax highlighting for rust,ignore code blocks
    - Fixed markdown table extraction in the metadata collector and
      fixed lint level output
    - Adding the additional information row for lints
    - Changed the website title to Clippy's lint list
    - Flexing the website for mobile users
    - Added (?) references for lint levels and groups
    - Making deprecated lints look dead
    - Removed JS code block language extraction in favor of a rust
      implementation `rust-clippy#7352`
    - Added the suspicious lint group to the lint list
    - Remove trailing whitespaces from index.html
    - Fix code highlighting
    - Use default value if the docVersion is empty

Co-authored-by: Philipp Krones <[email protected]>
@flip1995 flip1995 force-pushed the 7172-adapt-website-to-new-format branch from dbddce1 to 322a768 Compare July 28, 2021 13:04
@flip1995
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 28, 2021

📌 Commit 322a768 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jul 28, 2021

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Jul 28, 2021
…ednet,flip1995

Switch CI to new metadata collection

r? `@xFrednet`

Things we have to keep in mind:

- This removes the template files and the scripts used for deployment from the checkout. This was added in rust-lang#5517. I don't think we ever needed those there. Not sure though.
- ~~As a result, we can't remove the python scripts yet. We have to wait until this hits a stable Clippy release.~~ I'll just break the next stable deploy and do it by hand once.
- This should be merged together with rust-lang#7279. Me and `@xFrednet` will coordinate the switch
- ...?

I still have to try out some things:

- [x] Is it worth caching? Yes
- [x] ~~Is it worth to do a release build?~~ Nope
- [x] Does it actually work? With a few changes, yes
- [ ] ...?

changelog: Clippy now uses a lint to generate its documentation 🎉
bors added a commit that referenced this pull request Jul 28, 2021
Rollup of 3 pull requests

Successful merges:

 - #7279 (Adapting the lint list to Clippy's new metadata format)
 - #7298 (Switch CI to new metadata collection)
 - #7420 (Update lint documentation to use markdown headlines)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup

changelog: rollup
@bors bors merged commit ce46599 into rust-lang:master Jul 28, 2021
@xFrednet xFrednet deleted the 7172-adapt-website-to-new-format branch July 28, 2021 16:44
bors added a commit that referenced this pull request Jul 29, 2021
Updated changelog for 1.55

This has again been a bit of work, but I'm happy to notice that my English is still improving, and I'm getting faster at these things. That's a very nice side effect of contributing and getting feedback on reviews 😊

Moving on, there were a few things that I was unsure about:
* The PR rust-lang/rust#86717 changes an old entry in the change log, is this worth mentioning? I've left it out for now.
* The stabilization of `cargo clippy --fix` is quite awesome and important IMO. It sadly gets a bit lost in the *Other* entry, as it's the last one. Do we maybe want to move it somewhere else or change the headline order for this release?
* I've listed the introduction of the new `suspicious` group under the *Moves and Deprecations* section. Is this alright, or should it be moved to the *Other* section as well?
* Last but definitely not least, some fun! I've used the 🎉 emoji in the `cargo clippy --fix` entry, is this okay?

Sorry for the bombardment of questions xD

---

The PR already includes the entries for the new metadata collection and website updates. These are not merged yet, but should probably be to make this correct. This might also require the commit hashes to be updated (Not sure on this, though). It would actually be super fitting to get this into this release as we also stabilize `--fix`. TODOs:
* [x] Merge metadata collection PRs:
  1. #7279
  2. #7298
  3. #7420 (Hope to not get any merge conflicts)

---

[Rendered 📰](https://github.com/xFrednet/rust-clippy/blob/changelog-1-55/CHANGELOG.md)

r? `@flip1995`

changelog: none
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.

Website: Show which lints are auto-correctable with cargo fix link from the lint in the docs to the code
4 participants