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

Add allow(clippy::missing_docs_in_private_items) #356

Merged
merged 2 commits into from Jan 25, 2024

Conversation

daxpedda
Copy link
Contributor

Apparently clippy::missing_docs_in_private_items triggers in proc-macros, seeing that pin-project already adds a sizable amount of #[allow(...)]s I thought a PR might not hurt.

I also submitted an issue at Clippy: rust-lang/rust-clippy#12197.

@daxpedda daxpedda force-pushed the missing-docs-in-private-items branch 2 times, most recently from 49c7a47 to 5bf562b Compare January 25, 2024 12:28
@taiki-e
Copy link
Owner

taiki-e commented Jan 25, 2024

Thanks! Could you add a case that triggers this to tests/lint.rs test?

@daxpedda daxpedda force-pushed the missing-docs-in-private-items branch from 5bf562b to 75fa3f7 Compare January 25, 2024 13:18
@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 25, 2024

The lint apparently doesn't trigger in tests, so I can't add it in tests/lint.rs.
I tried adding it in a unit test, but that doesn't work either because the proc-macro can't find pin_project as a dependency.
Also tried to add pin_project as a dev-dependency, but that still requires me to put it behind a #[cfg(test)] guard, which again disables the lint.

In the meantime I found that empty enum variants can still trigger this lint, so I had to add another #[allow(...)].

Here is some code that can trigger both cases:

//! Testing `clippy::missing_docs_in_private_items`.

#![warn(clippy::missing_docs_in_private_items)]

use pin_project::pin_project;

#[pin_project(project_replace)]
pub struct Test1 {
    pub test: u8, // Triggered here.
}

#[pin_project(project = Test2Proj)]
pub enum Test2 {
    VariantA(u8),
    VariantB, // Triggered here.
}

@daxpedda
Copy link
Contributor Author

I guess one last idea to get this lint tested would be to make a sub-crate only for lint testing, so it's not behind a #[cfg(test)].
Could potentially also move the whole of tests/lint.rs there?

@taiki-e
Copy link
Owner

taiki-e commented Jan 25, 2024

I guess one last idea to get this lint tested would be to make a sub-crate only for lint testing, so it's not behind a #[cfg(test)].
Could potentially also move the whole of tests/lint.rs there?

I think this (convert the tests/lint.rs to a new test crate) is a reasonable idea. We already have such test crates to check compatibility with no-std/old edition.

@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 25, 2024

Done.
This also uncovered two more cases of FP that I addressed as well.

@daxpedda daxpedda force-pushed the missing-docs-in-private-items branch from 3624259 to a363a97 Compare January 25, 2024 15:06
@daxpedda daxpedda force-pushed the missing-docs-in-private-items branch from a363a97 to 5bc8a1a Compare January 25, 2024 15:16
Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks!

@taiki-e taiki-e merged commit 0b65093 into taiki-e:main Jan 25, 2024
10 checks passed
@taiki-e
Copy link
Owner

taiki-e commented Jan 25, 2024

Published in 1.1.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants