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

Empty docs #12342

Merged
merged 17 commits into from Feb 26, 2024
Merged

Empty docs #12342

merged 17 commits into from Feb 26, 2024

Conversation

lucarlig
Copy link
Contributor

@lucarlig lucarlig commented Feb 24, 2024

Fixes #9931

changelog: [empty_doc]: Detects documentation that is empty.
changelog: Doc comment lints now trigger for struct field and enum variant documentation

@rustbot
Copy link
Collaborator

rustbot commented Feb 24, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 24, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 24, 2024

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:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 24, 2024
@lucarlig
Copy link
Contributor Author

Not sure why the tests are failing, I haven’t contributed much maybe I could add suggestions as well

Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

The code mostly looks ok, though I'd like more tests and more explanation with regards to the implementation choices.

clippy_lints/src/empty_docs.rs Outdated Show resolved Hide resolved
tests/ui/empty_docs.rs Outdated Show resolved Hide resolved
clippy_lints/src/empty_docs.rs Outdated Show resolved Hide resolved
@bors
Copy link
Collaborator

bors commented Feb 24, 2024

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

clippy_lints/src/doc/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/doc/mod.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member

Mostly cleanup remaining but looks like you're almost done. 👍

Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

Looks good to me. One thing to note is some doc lints will be triggered on variants struct fields whereas they were not before.

@lucarlig
Copy link
Contributor Author

lucarlig commented Feb 26, 2024

Looks good to me. One thing to note is some doc lints will be triggered on variants struct fields whereas they were not before.

yeah I know, I was wondering if it was worth opening an issue to ensure that these cases are tested and working as intended. Probably all doc lints should apply to variants and fields, not only the ones in check_epxr.

Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

This looks much better. I have a small documentation nit, and we should amend the changelog. Otherwise this is ready to merge.

clippy_lints/src/doc/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/doc/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/doc/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/doc/mod.rs Show resolved Hide resolved
@llogiq
Copy link
Contributor

llogiq commented Feb 26, 2024

Thank you!

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 26, 2024

📌 Commit fca77c0 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Feb 26, 2024

⌛ Testing commit fca77c0 with merge 1c50948...

@bors
Copy link
Collaborator

bors commented Feb 26, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 1c50948 to master...

1 similar comment
@bors
Copy link
Collaborator

bors commented Feb 26, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 1c50948 to master...

@bors bors merged commit 1c50948 into rust-lang:master Feb 26, 2024
8 checks passed
@bors
Copy link
Collaborator

bors commented Feb 26, 2024

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

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.

Detect documentation that is empty
6 participants