rustdoc: add doc comment to DocVisitor #130537
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
src/librustdoc/visit.rs
Outdated
| @@ -1,5 +1,11 @@ | |||
| use crate::clean::*; | |||
|
|
|||
| /// allows a type to traverse the cleaned as of a crate. | |||
There was a problem hiding this comment.
Nit:
| /// allows a type to traverse the cleaned as of a crate. | |
| /// Allows a type to traverse the cleaned AST of a crate. |
There was a problem hiding this comment.
you want me to capitalize this sentence specifically? and not any of the other doc comments?
i was just matching the style of the existing comments.
There was a problem hiding this comment.
Sorry ignore the capitalization if it's pre-existing, just the as -> AST
lolbinarycat
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #130857) made this pull request unmergeable. Please resolve the merge conflicts. |
lolbinarycat
force-pushed
the
rustdoc-DocVisitor-docs
branch
from
dc9c4ad
to
8aa6dd6
Compare
| /// note that like [`rustc_ast::visit::Visitor`], but | ||
| /// unlike [`rustc_lint::EarlyLintPass`], if you override a | ||
| /// `visit_*` method, you will need to manually recurse into | ||
| /// its contents. |
There was a problem hiding this comment.
content is never plural iirc
There was a problem hiding this comment.
"contents" is definitely a word, it implies there is likely to be more than one thing contained.
eg. table of contents
There was a problem hiding this comment.
https://dictionary.cambridge.org/fr/grammaire/grammaire-britannique/content-or-contents
Seems tricky. So I think either way is fine.
src/librustdoc/visit.rs
Outdated
| @@ -1,5 +1,11 @@ | |||
| use crate::clean::*; | |||
|
|
|||
| /// allows a type to traverse the cleaned ast of a crate. | |||
There was a problem hiding this comment.
Missing capitalized letter.
There was a problem hiding this comment.
There was a problem hiding this comment.
Please do it. Comments should be capitalized.
There was a problem hiding this comment.
Is that an official policy? checking the rustc dev guide, the std dev guide, and the api docs conventions rfc, there's no comment on capitalization preference.
currently deferring to previous approval, and the golden rule of code formatting (match the existing formatting of the file you're editing).
There was a problem hiding this comment.
And you'll see across the files that both styles are there. So please use the one with capital and even send a PR to fix the rest if you want.
There was a problem hiding this comment.
the current file (visit.rs) only uses lowercase. this is mainly because a lot of its doc comments are not full sentences.
in our postmodern english, capitalizing a sentence fragment is arguably worse than not capitalizing a full sentence. to make these docs fully idiomatic, i would need to rephrase all the sentence fragments into full sentences, which isn't trivial.
really, the comments on the trait methods could be more helpful, but i don't have the expertise to fully explain their purpose.
perfect is the enemy of good.
There was a problem hiding this comment.
You realize that doing what I'm asking would have allowed this PR to have been merged a lot earlier, right?
There was a problem hiding this comment.
It probably could've been merged faster if you were less insistent on enforcing non-existent style guide rules, but if you really care that much, i guess i can capitalize it.
There was a problem hiding this comment.
honestly there probably should be at least a suggestion to use proper capitalization, expecially for official documentation.
enforcing an unwritten rule still feels pretty bad.
There was a problem hiding this comment.
I'm used to have sentences start with a capital letter and end with a dot. The only cases where I don't enforce it is on big PRs where the comments are finally just a small part of and I don't want to slow the merge.
src/librustdoc/visit.rs
Outdated
| @@ -1,5 +1,11 @@ | |||
| use crate::clean::*; | |||
|
|
|||
| /// allows a type to traverse the cleaned ast of a crate. | |||
| /// | |||
| /// note that like [`rustc_ast::visit::Visitor`], but | |||
There was a problem hiding this comment.
Missing capitalized letter.
src/librustdoc/visit.rs
Outdated
| @@ -58,6 +64,7 @@ pub(crate) trait DocVisitor<'a>: Sized { | |||
| m.items.iter().for_each(|i| self.visit_item(i)) | |||
| } | |||
|
|
|||
| /// the main entrypoint of [`DocVisitor`]. | |||
There was a problem hiding this comment.
Missing capital here too.
There was a problem hiding this comment.
intentional stylistic choice to match existing doc comments.
lolbinarycat
force-pushed
the
rustdoc-DocVisitor-docs
branch
from
8aa6dd6
to
5ef63ec
Compare
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
…llaumeGomez Rollup of 5 pull requests Successful merges: - rust-lang#130383 (check if it's rust-lang/rust CI job in `llvm::is_ci_llvm_modified`) - rust-lang#130416 (resolve rust-lang#130122: reword 'sort-by' edge-conditions documentation) - rust-lang#130537 (rustdoc: add doc comment to DocVisitor) - rust-lang#130743 (Clarifications for set_nonblocking methods) - rust-lang#131010 (extend comment in global_llvm_features regarding target-cpu=native) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#130537 - lolbinarycat:rustdoc-DocVisitor-docs, r=GuillaumeGomez rustdoc: add doc comment to DocVisitor
No description provided.