Skip to content

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

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.

7 participants