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

Document lint configuration values in Clippy's book #10199

Merged
merged 7 commits into from Jan 14, 2023

Conversation

tylerjw
Copy link
Contributor

@tylerjw tylerjw commented Jan 13, 2023

changelog: document lint configuration values in Clippy's book

fixes #9991

r? @xFrednet

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 13, 2023
@tylerjw
Copy link
Contributor Author

tylerjw commented Jan 13, 2023

To note, the reason for removing a reference to cognitive-complexity-threshold in the docs is that it encourages the use of a check in the Nursery because it is not well documented.

@tylerjw tylerjw changed the title Document extending list type configs Document lint configuration values in Clippy's book Jan 13, 2023
@tylerjw tylerjw force-pushed the document-extending-list-configs branch from b6c67e9 to 1edac66 Compare January 13, 2023 18:40
@tylerjw
Copy link
Contributor Author

tylerjw commented Jan 13, 2023

@xFrednet I believe I'm ready for a first review. Ok, now I am, fixed up the tests.

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
@tylerjw tylerjw force-pushed the document-extending-list-configs branch from 1edac66 to 2e2ae68 Compare January 13, 2023 18:48
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

A solid start, thank you for the work! I've added some comments, and also gave some feedback about the table/format in the issue. I like the look of the table, but this might be a case where a different format is sadly more functional.

README.md Outdated Show resolved Hide resolved
book/src/lint_configuration.md Outdated Show resolved Hide resolved
.github/workflows/remark.yml Outdated Show resolved Hide resolved
@tylerjw
Copy link
Contributor Author

tylerjw commented Jan 13, 2023

I updated this to produce both a table and paragraph sections. The table portion looks like this:

image

And then if you were to click on any of those configs it would go to a table section that looks like this:

image

I tried making the table widths work for the names, default values, and lints but couldn't get it to look good, so I moved the lints into the paragraphs and made the table just the config names and their default values. Clicking on any one goes to the paragraph, which now includes not only the list of lints but also the config type and the whole doc string.

@tylerjw tylerjw force-pushed the document-extending-list-configs branch 5 times, most recently from 0bc745d to ce75ce8 Compare January 13, 2023 23:24
Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
@tylerjw tylerjw force-pushed the document-extending-list-configs branch from ce75ce8 to 7d1609d Compare January 13, 2023 23:26
@tylerjw
Copy link
Contributor Author

tylerjw commented Jan 13, 2023

@xFrednet this is ready for another review. I made some changes to how the generated doc output. Let me know if you have any ideas for how I could format it better and other things you notice that I could improve.

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

This version looks good to me! I have two more required adjustments, and then we can merge this.

  1. Is the comment for the CI changes
  2. Could you also update the add configuration to a lint section to have a 5th step, to run cargo collect-metadata?

Then this should be ready! Thank you very much for this update!!!!

.github/workflows/remark.yml Outdated Show resolved Hide resolved
@tylerjw
Copy link
Contributor Author

tylerjw commented Jan 14, 2023

@xFrednet I pushed the two changes you laid out. Thank you for going and figuring out the github actions magic I needed for this.

@xFrednet
Copy link
Member

Everything looks perfect to me! Thank you very much. This is a very nice step forward for Clippy's configuration!

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 14, 2023

📌 Commit c0da8ac has been approved by xFrednet

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jan 14, 2023

⌛ Testing commit c0da8ac with merge aceb443...

@bors
Copy link
Collaborator

bors commented Jan 14, 2023

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

@bors bors merged commit aceb443 into rust-lang:master Jan 14, 2023
@tylerjw tylerjw deleted the document-extending-list-configs branch January 14, 2023 18:29
@flip1995
Copy link
Member

Awesome! Thanks for implementing this :)

@xFrednet Sorry for not replying to your CI question. But the solution you came up with is perfect 👍

@xFrednet
Copy link
Member

@flip1995 No problem at all, I would have waited if Google didn't turn up such a simple solution. :)

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.

Document lint configuration values in Clippy's book
5 participants