-
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
WIP: empty_doc #11633
WIP: empty_doc #11633
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Alexendoo (or someone else) soon. 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 (
|
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.
Very good for a first patch ❤️, mainly just some documentation rephrasing and refactoring to fit better with the rest of the codebase 🐈
tests/ui/empty_docs.rs
Outdated
//! | ||
|
||
//! valid doc comment | ||
|
||
//!! | ||
|
||
//!! valid doc comment | ||
|
||
/// | ||
|
||
/// valid doc comment | ||
|
||
/** | ||
* | ||
*/ | ||
|
||
/** | ||
* valid block doc comment | ||
*/ | ||
|
||
pub mod inner_module {} |
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.
None of these are actually empty documentation, this is ~equivalent to
//!
//! valid doc comment
//!!
//!! valid doc comment
///
/// valid doc comment
///
/// valid block doc comment
Because doc comments and #[doc]
attributes are merged together
This really should go in doc.rs
where this merging/parsing is all handled already
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.
@Alexendoo according to https://doc.rust-lang.org/reference/comments.html there are one line and block doc comments.
What you show in your example is 8 separate one-line doc comments. They are not being merged in 2, just because there are no line breaks within them. So lint in this PR will trigger on lines 1, 3, 5, 6 (and I'm updating the test to demonstrate this).
As for block doc comments, I assume they might have empty lines (for formatting purposes) because the definition of empty
for a block comment is when all lines are empty.
Tbh I do not understand the part about merging with #[doc]
attributes. Can you please explain?
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.
This really should go in doc.rs where this merging/parsing is all handled already
there is no much merging/parsing happening in this current lint. The idea is to have all doc-related lints in doc.rs?
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.
I'm not saying the attributes are merged in the AST/HIR, but the multiple attributes representing doc comments are merged into a single piece of documentation by rustdoc, be they line comments, block comments or #[doc = ".."]
attributes
My example was to represent what rustdoc approximately sees as the documentation, not to say that doc comments get converted to line comments. A screenshot may be clearer:
///
/// valid doc comment
/**
*
*/
/**
* valid block doc comment
*/
pub mod inner_module {}
The whitespace between the doc comments are not significant, e.g.
/// a
///
/// b
is the same as
/// a
///
/// b
But we certainly don't want to lint the middle line comment, it's significant because it causes a
and b
to be separate paragraphs
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.
there is no much merging/parsing happening in this current lint. The idea is to have all doc-related lints in doc.rs?
Yeah, it could go here
rust-clippy/clippy_lints/src/doc.rs
Lines 493 to 495 in 3662bd8
if doc.is_empty() { | |
return Some(DocHeaders::default()); | |
} |
With .is_empty()
being changed to check that doc
only contains whitespace if needed, you can use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_resolve/rustdoc/fn.span_of_fragments.html on fragments
to get the span
remove empty line Co-authored-by: Alejandra González <blyxyas@gmail.com>
Co-authored-by: Alejandra González <blyxyas@gmail.com>
Co-authored-by: Alejandra González <blyxyas@gmail.com>
Co-authored-by: Alejandra González <blyxyas@gmail.com>
There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged. You can start a rebase with the following commands:
The following commits are merge commits: |
☔ The latest upstream changes (presumably #11791) made this pull request unmergeable. Please resolve the merge conflicts. |
comment | ||
.trim() | ||
.split("\n") | ||
.map(|comment| comment.trim().trim_matches('*').trim_matches('!')) |
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.
I think that one shouldn't be needed.
.trim() | ||
.split("\n") | ||
.map(|comment| comment.trim().trim_matches('*').trim_matches('!')) | ||
.collect::<Vec<&str>>() |
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.
I think there is a missing .filter(|line| !line.is_empty())
Looks like this was continued in #12342 because of inactivity. The lint exists now: https://rust-lang.github.io/rust-clippy/master/index.html#/empty_docs |
@xFrednet hi, I do not plan to continue work on this implementation. Happy that someone finally did this ) |
Fixes #9931