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

EmptyDocs #10152

Closed
wants to merge 5 commits into from
Closed

EmptyDocs #10152

wants to merge 5 commits into from

Conversation

cgorski
Copy link

@cgorski cgorski commented Jan 3, 2023

WIP check


changelog: new lint: [empty_docs]
#10152

@rustbot
Copy link
Collaborator

rustbot commented Jan 3, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @giraffate (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 3, 2023
@cgorski
Copy link
Author

cgorski commented Jan 3, 2023

This is my first time contributing and I sent this pull request to get feedback on #9931

}
}

fn process_attributes(self, ex: &EarlyContext<'_>, parent_span: Span, attributes: &Vec<Attribute>) {
Copy link
Member

Choose a reason for hiding this comment

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

We have a similar clippy util to extract comments, although it takes all comments, not just doc coms.

Will something like that help here?

Copy link
Member

Choose a reason for hiding this comment

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

We have some code that reassembles doc comments for some related lints, the lint could be moved into doc.rs and check here-ish based on spans + doc

Sorry to point that out after you've done the complicated part yourself already 😬

Not the first time this has happened with doc.rs, maybe it would be worth renaming it to doc_comments.rs or something, but it'll still be easy to miss

@@ -4,6 +4,26 @@ version = "0.1.68"
edition = "2021"
publish = false

[target.'cfg(NOT_A_PLATFORM)'.dependencies]
Copy link
Member

Choose a reason for hiding this comment

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

Is this an accidental commit?

Comment on lines +137 to +141
if let Some(segment) = normal_attr.item.path.segments.get(0) {
if segment.ident.as_str() == "doc" { true } else { false }
} else {
false
}
Copy link
Member

Choose a reason for hiding this comment

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

style nits:

normal_attr.item.path.segments.get(0).map_or(false, |segment| segment.ident.as_str == "doc")

Comment on lines +148 to +156
error: invalid `doc` attribute
--> $DIR/empty_docs.rs:5:7
|
LL | #[doc("this is a doc")]
| ^^^^^^^^^^^^^^^
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #82730 <https://github.com/rust-lang/rust/issues/82730>
= note: `-D invalid-doc-attributes` implied by `-D warnings`
Copy link
Member

Choose a reason for hiding this comment

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

I personally think we can just not cover this scenario in this lint since this is being phased out. The compiler will warn anyway.

@dswij
Copy link
Member

dswij commented Jan 11, 2023

Thanks for taking the time to open this PR, and welcome to clippy!

I understand that this is a WIP, but I left some initial comments. Feel free to reach out to any clippy members if there is any questions

@cgorski
Copy link
Author

cgorski commented Jan 13, 2023

Thanks for the feedback! I’m out for holiday this weekend but I’ll resume work next week.

@bors
Copy link
Contributor

bors commented Jan 19, 2023

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

@blyxyas
Copy link
Member

blyxyas commented Oct 6, 2023

Closing this since it has been stale for 266 days.
Thanks for trying! ❤️

@blyxyas blyxyas closed this Oct 6, 2023
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.

8 participants