-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Clippy book #7359
Clippy book #7359
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks already way better than the previous attempt. This is a good starting point in working out how the book should look like cc @rust-lang/clippy
Some things that should be done in this PR (not necessarily by you):
- There should also exist documentation on how to build the book (you can put this in "Development" or "Infrastructure" section)
- Our automation probably have to be adapted to this (not sure what exactly yet, but I'd assume
clippy_dev
and the metadata collection, depending on what we want to do with this)
2. A rough outline of Clippy lint categories
I like this. I even would like to put an automation in place that puts all lints from the specific category in those files, linking to the documentation with a short description of the lint. (We should first switch to the new metadata collection though #7298)
1. is `guide/` the right place? I'm modeling after mdbook itself.
Why not book/
?
2. What is the relationship between ALL the Clippy Lints and this guide?
TBH we're not quite sure yet. For now they should coexist. For your other questions, see above.
3. Related to the above, where should this guide be published since the `gh-pages` branch is already in use?
Good question. Probably also on the gh-pages branch, but add a link on the website (in addition to the current README
) to the book. We can just push the artifacts to a book/
dir on the gh-pages
branch.
4. This PR doesn't currently change any existing content.
That's fine. But we should rewrite the README and remove the duplicated documentation int doc/
. The README should probably only contain a link to the book and a quick-start guide to Clippy. That quick-start guide should probably mention lint groups, how to allow/deny lints and link to the book for things like how to configure Clippy.
7fc1159
to
2882a60
Compare
afd77d1
to
25e5fae
Compare
@joshrotenberg I want to first get our infra move through, before I can focus on this PR, since this kind of depends on it¹. So expect that it will take about 2 weeks before we can really make progress with this PR. I hope that's ok with you. Also, since I expect that this PR will include many incremental changes, could you avoid amending the one commit in this PR and just add a new commit to change something? Also please only force push if you have to do a rebase. That will make reviewing and working on this way easier. ¹: I want to get some automation in it in this PR already and we would do the work twice if we'd do it with the current infra and then again for the new one. |
No problem. I can leave it as is if you like, and then we can discuss more when it's a good time to make more progress.
Sure thing. I've gotten into the habit of squashing to keep things cleaned up but I'm happy to follow whatever process works for this.
Makes sense. |
Good news: The metadata collector move is done, so getting this through will be my next project. 🚀 |
Great! I'd love to get back to this! I made a few minor updates just the other day. Going back through some of the comments above: I added a short blurb about how to do this. I'm sure it can be improved. I think your idea about just publishing to There is also the question of the And then removing duplicate content from the I'm happy to work on, collaborate and/or relinquish any of the above, just let me know your thoughts. |
I wouldn't include this in this PR, but take care of this once the book is merged. This was also a successful approach with the metadata collector. First implement everything and then switch the infra to that.
I would leave it out for this PR, but I'd like a list of the lints in the book later. The primary documentation should live on the website though. So a static page linking to the website should be good for now.
This should be a quick thing to do, so I would do this once we move everything over to the book. I still have to review this PR in-depth. Thanks for all the work you've put into this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of review with suggestions to better structure this book. Let me know if you disagree with anything!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next round of review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments. Please also fix this: #7359 (comment)
After that, I think I'll write some text myself for the currently empty chapters and then this should be good to go.
I'll summarize all the open points that are left in this PR, to have it in one place instead of spread out over multiple review comment threads. I'll still have to figure out what to address in this PR and what can be addressed later.
I I want to get this book into the repo in a "first release" state instead of a "first draft" state. That lessens the the pressure of directly opening and merging follow up PRs once this gets merged. I'll address the above points myself, so nothing to do for you @joshrotenberg for now. 👍 |
Sorry, I've been on vacation for a bit. I'm back now, feel free to throw anything my way on this if you'd like. |
It's mostly stuff I have to take a look at myself first. But once I got to it and find things, I can throw your way, I'll definitely do so! |
Just checking in to see if I can help out here. |
I didn't had time to get to it. I started working on it, but just for a few hours, so there's still a bit to do for me, before this can move forward. I plan to address this over the holidays and now interpret me working on this as my Christmas present for Clippy 😄 |
I didn't manage to work on this over the holidays. I was too busy enjoying my vacation. I have some more time I can spend on Clippy this month, so I try to get this out of the backlog. |
Hope you had a great vacation! As always, let me know if I can help out here. |
277cd58
to
673bd0c
Compare
Alright, I did some work (still not finished though). I also updated my todo comment from above accordingly. I rebased this branch on the latest master (clean rebase, no changes of Josh's commits). |
☔ The latest upstream changes (presumably #8359) made this pull request unmergeable. Please resolve the merge conflicts. |
673bd0c
to
36cfd87
Compare
This reformats all the internal docs, so that the md files use at most 80 characters per line. This is the usual formatting of md files. We allow 120 chars per line in CI though.
The UI tests now use the latest edition by default. Testing on older editions should almost never be necessary, so I don't see a need to document this.
This removes the empty separate files for the different groups and adds a single file giving short explanations for each group and what to expect from those groups.
Group everything that has something to do with Clippy development under the "Development" chapter, so that Clippy users can't get confused.
Recommend the -Dwarnings and --all-targets/--all-features more strongly.
- Move doc about defining remotes to the front and use the defined remotes in the further documentation - Don't recommend pushing to the remote repo directly - Add some clarifying comments
Make it clear for all code blocks in which repository they should be run. Also make sure that the correct beta commit is fetched.
I squashed a bunch of commits and rebased on the latest master without any changes to the last state you just reviewed. (except for the conflict resolution in adding_lints and common_tools of course). Do you think I should quash more commits or is this ok like it is? If it is ok, you can r+ it and we can finally work on publishing the book :) |
The number of commits looks reasonable to me and there are no filler ones, so it's fine. I guess this marks the beginning of an era ^^ A big thank you to @joshrotenberg for taking the initiative and starting to write the book. And thank you, @flip1995, for reviewing the first part, adding new chapters and applying my suggestions. Without further of do, I'll hand this off to our merging wizard bors: @bors r+ |
📌 Commit b2660de has been approved by |
Clippy book A work in progress Clippy Book using mdbook. See #6011. This is currently just a moving around of things: 1. The current README.md split up a bit and put into sections. 1. A rough outline of Clippy lint categories (currently no content, potentially add a basic introduction for each and some example, see questions below. 1. The `docs` content repurposed into a top level `Development` section. 1. The current Roadmap. Some big questions: 1. is `guide/` the right place? I'm modeling after mdbook itself. 1. What is the relationship between ALL the Clippy Lints and this guide? It seems like they can coexist. Does that mean the guide should just point to the current side with regard to actual lints, and maybe just include some examples to keep it interesting? Keeping both up to date seems like a maintenance nightmare unless its automated somehow. Or should the current ALL the Clippy lints somehow be incorporated into the book? 1. Related to the above, where should this guide be published since the `gh-pages` branch is already in use? 1. This PR doesn't currently change any existing content. Obviously that would make sense assuming the general structure and relocation is an acceptable approach. --- Open Tasks for follow up PR: - Set up CI/CD - Split up Installation and Usage - Add more content to Usage (and Installation) chapters - Enhance CI chapter with more examples for different CIs
💔 Test failed - checks-action_test |
The changelog entry was missing, I've added one @bors retry |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
A work in progress Clippy Book using mdbook. See #6011.
This is currently just a moving around of things:
docs
content repurposed into a top levelDevelopment
section.Some big questions:
guide/
the right place? I'm modeling after mdbook itself.gh-pages
branch is already in use?Open Tasks for follow up PR:
changelog: The first version of the Clippy Book
staring, The Clippy Team, Bors and rustc