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

Clippy book #7359

Merged
merged 19 commits into from
Jun 6, 2022
Merged

Clippy book #7359

merged 19 commits into from
Jun 6, 2022

Conversation

joshrotenberg
Copy link
Contributor

@joshrotenberg joshrotenberg commented Jun 16, 2021

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.
  2. A rough outline of Clippy lint categories (currently no content, potentially add a basic introduction for each and some example, see questions below.
  3. The docs content repurposed into a top level Development section.
  4. The current Roadmap.

Some big questions:

  1. is guide/ the right place? I'm modeling after mdbook itself.
  2. 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?
  3. Related to the above, where should this guide be published since the gh-pages branch is already in use?
  4. 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

changelog: The first version of the Clippy Book
staring, The Clippy Team, Bors and rustc

@rust-highfive
Copy link

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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 16, 2021
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.

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.

guide/src/SUMMARY.md Outdated Show resolved Hide resolved
guide/src/SUMMARY.md Outdated Show resolved Hide resolved
guide/src/configuration.md Outdated Show resolved Hide resolved
@flip1995
Copy link
Member

@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.

@joshrotenberg
Copy link
Contributor Author

@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.

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.

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.

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.

¹: 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.

Makes sense.

@flip1995
Copy link
Member

Good news: The metadata collector move is done, so getting this through will be my next project. 🚀

@joshrotenberg
Copy link
Contributor Author

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 gh-pages into a book directory and linking from the index of what is currently there makes sense. I took a brief look at the deploy.yml: maybe it's just another step in there?

There is also the question of the Lints section in the book: a) make it static content with more of an overview (with links to the Clippy Lints site), b) do something dynamic there like you mentioned, c) leave it out for now since Clippy Lints has everything someone might need anyway.

And then removing duplicate content from the README.md and the doc directory: remove it in this PR? Get everything else squared away and do another PR to migrate?

I'm happy to work on, collaborate and/or relinquish any of the above, just let me know your thoughts.

@flip1995
Copy link
Member

flip1995 commented Jul 30, 2021

I think your idea about just publishing to gh-pages into a book directory and linking from the index of what is currently there makes sense.

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.

c) leave it out for now

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.

And then removing duplicate content from the README.md and the doc directory

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!

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 round of review with suggestions to better structure this book. Let me know if you disagree with anything!

book/src/infrastructure/book.md Outdated Show resolved Hide resolved
book/src/infrastructure/book.md Outdated Show resolved Hide resolved
book/src/README.md Outdated Show resolved Hide resolved
book/src/installation_and_usage.md Outdated Show resolved Hide resolved
book/src/installation_and_usage.md Outdated Show resolved Hide resolved
book/src/configuration.md Show resolved Hide resolved
book/src/configuration.md Show resolved Hide resolved
book/src/development/README.md Show resolved Hide resolved
book/src/development/basics.md Show resolved Hide resolved
book/src/roadmap/2021.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.

Next round of review.

book/src/continuous_integration/github_actions.md Outdated Show resolved Hide resolved
book/src/continuous_integration/github_actions.md Outdated Show resolved Hide resolved
.github/workflows/remark.yml Outdated Show resolved Hide resolved
book/src/development/basics.md Outdated Show resolved Hide resolved
book/src/installation_and_usage.md Outdated Show resolved Hide resolved
book/src/configuration.md Outdated Show resolved Hide resolved
book/src/continuous_integration/travis.md Outdated Show resolved Hide resolved
@joshrotenberg joshrotenberg marked this pull request as ready for review August 5, 2021 03:22
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.

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.

book/book.toml Outdated Show resolved Hide resolved
book/book.toml Outdated Show resolved Hide resolved
book/book.toml Show resolved Hide resolved
@flip1995
Copy link
Member

flip1995 commented Aug 25, 2021

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.

  • Clean up / split up CONTRIBUTING.md and move the contents into the book (possibly new sections/chapters)
  • Add text to chapter introductions (Possibly from CONTRIBUTING.md)
    • Clippy Development
    • Infrastructure
    • Roadmap
    • Clippy Lints (maybe remove the draft sections for now)
    • CI (Also general cleanup in the whole chapter)
  • Figure out what to change in clippy_dev
    • Just make sure to auto-update the "There are over X lints in Clippy" line in the Introduction chapter
  • Move doc/ contents completely to the book instead of copying them
  • Split up Installation and Usage (those probably will need a rewrite)
  • Move note from "Travis CI" section to usage and link to it from each CI section
  • Enhance CI chapter with more examples for different CIs
  • Set up CI/CD for deployment of the book
  • Slim down the README.md and just link to the book.

I checked off quoted the once I think can/should be done in follow up PRs


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. 👍

@joshrotenberg
Copy link
Contributor Author

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.

@flip1995
Copy link
Member

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!

@joshrotenberg
Copy link
Contributor Author

Just checking in to see if I can help out here.

@flip1995
Copy link
Member

flip1995 commented Nov 16, 2021

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 😄

@flip1995
Copy link
Member

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.

@joshrotenberg
Copy link
Contributor Author

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.

@flip1995
Copy link
Member

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).

@bors
Copy link
Collaborator

bors commented Jan 27, 2022

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

joshrotenberg and others added 18 commits June 6, 2022 16:15
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.
@flip1995
Copy link
Member

flip1995 commented Jun 6, 2022

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 :)

@xFrednet
Copy link
Member

xFrednet commented Jun 6, 2022

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+

@bors
Copy link
Collaborator

bors commented Jun 6, 2022

📌 Commit b2660de has been approved by xFrednet

bors added a commit that referenced this pull request Jun 6, 2022
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
@bors
Copy link
Collaborator

bors commented Jun 6, 2022

⌛ Testing commit b2660de with merge 572afbe...

@bors
Copy link
Collaborator

bors commented Jun 6, 2022

💔 Test failed - checks-action_test

@xFrednet
Copy link
Member

xFrednet commented Jun 6, 2022

The changelog entry was missing, I've added one

@bors retry

@bors
Copy link
Collaborator

bors commented Jun 6, 2022

⌛ Testing commit b2660de with merge 72f5ff6...

@bors
Copy link
Collaborator

bors commented Jun 6, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 72f5ff6 to master...

@bors bors merged commit 72f5ff6 into rust-lang:master Jun 6, 2022
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