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.

Sign up for GitHub

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

Jump to bottom

rustdoc: Clarified the attribute which prompts the warning #85223

Merged
merged 4 commits into from
Oct 2, 2021
Merged

rustdoc: Clarified the attribute which prompts the warning #85223

merged 4 commits into from
Oct 2, 2021

Conversation

simbleau
Copy link
Contributor

The example call was lacking clarification of the #![warn(rustdoc::invalid_codeblock_attributes)] attribute which generates the specified warning.

@simbleau
Clarified the attribute which prompts the warning
b65bad9 
The example call was lacking clarification of the  `#![warn(rustdoc::invalid_codeblock_attributes)]` attribute which generates the specified warning.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (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 May 12, 2021
@jyn514 jyn514 changed the title Doc: Clarified the attribute which prompts the warning rustdoc: Clarified the attribute which prompts the warning May 12, 2021
@jyn514
Copy link
Member

jyn514 commented May 12, 2021

It already shows it here: https://github.com/rust-lang/rust/pull/85223/files#diff-13b2c983ddb283f6812f85647105aaa7a89f2a697890095f02b9bbf82cf033ffR253
Why show it again?

Also, i don't think we should do this just this one lint, either all of them should have warn or none (my preference is none).

@simbleau
Copy link
Contributor Author

simbleau commented May 12, 2021 •
edited
Loading

It already shows it here: https://github.com/rust-lang/rust/pull/85223/files#diff-13b2c983ddb283f6812f85647105aaa7a89f2a697890095f02b9bbf82cf033ffR253
Why show it again?

Also, i don't think we should do this just this one lint, either all of them should have warn or none (my preference is none).

This is one of 2 outliers without the attribute specified to the user. Every other example specifies the exact attribute in the examples given. It seems more verbose to the user, especially to notify users of the prepend rustdoc::. The other exception is broken_intra_doc_links, which is related. I will append a commit for that example, too.

@simbleau
Clarified all attributes which prompt warnings
2b27c82 
All calls which trigger rustdoc warnings and are now properly verbose for consistency. This uses the attribute in the examples which provides the user with more context.
jyn514
jyn514 suggested changes May 16, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

You said

This is one of 2 outliers without the attribute specified to the user. Every other example specifies the exact attribute in the examples given.

But that doesn't seem to be true? The only attributes I see are for demonstrating how to allow/warn/deny, or for turning on lints that are allowed by default.

src/doc/rustdoc/src/lints.md Outdated Show resolved Hide resolved
src/doc/rustdoc/src/lints.md Outdated Show resolved Hide resolved
@simbleau
Noted necessessity and fixed rustdoc:: prefix
d7884d4 
Now shows that certain warnings are unnecessary but includes them for consistency.
@simbleau
Fixed item typo which did not need prefix
8a7bb2b
@simbleau
Copy link
Contributor Author

What you said is true; I included them for clarity but the attribs are unnecessary due to their defaults.
I opted to include them still (for consistency) but added a comment noting their necessity (for clarity).

Please review.

@jyn514
Copy link
Member

jyn514 commented May 16, 2021

I don't have time to finish this up, but @steveklabnik is assigned and he's a good reviewer for this :)

@bstrie bstrie added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 2, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2021
@Dylan-DPC-zz
Copy link

@steveklabnik any updates?

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2021
@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 17, 2021
@jackh726 jackh726 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 23, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@steveklabnik any updates?

@JohnCSimon
Copy link
Member

Ping again from triage:
@steveklabnik any updates?

@steveklabnik
Copy link
Member

Hi! So sorry, I don't know why but GitHub did not place this into my "reviews requested" notifications, and so I didn't realize I was the reviewer here.

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Oct 1, 2021

📌 Commit 8a7bb2b has been approved by steveklabnik

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 1, 2021
@Manishearth Manishearth mentioned this pull request Oct 1, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 1, 2021
@bors
Auto merge of rust-lang#89449 - Manishearth:rollup-3alb61f, r=Manishe…
6e12110 
…arth

Rollup of 7 pull requests

Successful merges:

 - rust-lang#85223 (rustdoc: Clarified the attribute which prompts the warning)
 - rust-lang#88847 (platform-support.md: correct ARMv7+MUSL platform triple notes)
 - rust-lang#88963 (Coerce const FnDefs to implement const Fn traits )
 - rust-lang#89376 (Fix use after drop in self-profile with llvm events)
 - rust-lang#89422 (Replace whitespaces in doctests' name with dashes)
 - rust-lang#89440 (Clarify a sentence in the documentation of Vec (rust-lang#84488))
 - rust-lang#89441 (Normalize after substituting via `field.ty()`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit eedc76d into rust-lang:master Oct 2, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 2, 2021
tniessen added a commit to tniessen/rust that referenced this pull request Oct 2, 2021
@tniessen
Fix typos in rustdoc/lints
e017e45 
Refs: rust-lang#85223
@tniessen tniessen mentioned this pull request Oct 2, 2021
Merged
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
@Manishearth
Rollup merge of rust-lang#89467 - tniessen:rustdoc-unecessary, r=jyn514
58c60a8 
Fix typos in rustdoc/lints

This PR merely fixes a few typos in a recently introduced change :)

Refs: rust-lang#85223
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
@Manishearth
Rollup merge of rust-lang#89467 - tniessen:rustdoc-unecessary, r=jyn514
8c7c689 
Fix typos in rustdoc/lints

This PR merely fixes a few typos in a recently introduced change :)

Refs: rust-lang#85223
flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 7, 2021
@bors
Auto merge of rust-lang#89449 - Manishearth:rollup-3alb61f, r=Manishe…
2cb37a1 
…arth

Rollup of 7 pull requests

Successful merges:

 - rust-lang#85223 (rustdoc: Clarified the attribute which prompts the warning)
 - rust-lang#88847 (platform-support.md: correct ARMv7+MUSL platform triple notes)
 - rust-lang#88963 (Coerce const FnDefs to implement const Fn traits )
 - rust-lang#89376 (Fix use after drop in self-profile with llvm events)
 - rust-lang#89422 (Replace whitespaces in doctests' name with dashes)
 - rust-lang#89440 (Clarify a sentence in the documentation of Vec (rust-lang#84488))
 - rust-lang#89441 (Normalize after substituting via `field.ty()`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 requested changes

@steveklabnik steveklabnik steveklabnik approved these changes

Assignees

@steveklabnik steveklabnik

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.