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

*: reduce all "deny" and "forbid" lints to "warn" #7692

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

benesch
Copy link
Member

@benesch benesch commented Aug 4, 2021

Lints at the deny/forbid level can be very frustrating when developing,
because you're forced to comply with the lint even when you're still
prototyping, just to get your code to compile.

This commit downgrades all these lints to "warn", to optimize for the
development experience. In CI, these lints are still enforced at the
"deny" level by our Clippy configuration, which upgrades all warnings to
hard errors.

The one glitch here is that forbid used to prevent submodules from
re-enabling the forbidden lint. I propose that we solve this socially,
by making it taboo to add #![allow(missing_docs)] unless you've gotten
buy-in from the overall owner of that area of the codebase.

Copy link
Contributor

@antifuchs antifuchs left a comment

Choose a reason for hiding this comment

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

Seeing as how I already do this for dataflow in #7690, I'm ok with this! The "no turning the level down" check does hurt a bit to lose, but this might be an interesting skunkworks project (;

Lints at the deny/forbid level can be very frustrating when developing,
because you're forced to comply with the lint even when you're still
prototyping, just to get your code to compile.

This commit downgrades all these lints to "warn", to optimize for the
development experience. In CI, these lints are still enforced at the
"deny" level by our Clippy configuration, which upgrades all warnings to
hard errors.

The one glitch here is that `forbid` used to prevent submodules from
re-enabling the forbidden lint. I propose that we solve this socially,
by making it taboo to add `#![allow(missing_docs)]` unless you've gotten
buy-in from the overall owner of that area of the codebase.
@frankmcsherry
Copy link
Contributor

I propose that we solve this socially, by making it taboo to add #![allow(missing_docs)] unless you've gotten buy-in from the overall owner of that area of the codebase.

We could remove the four occurrences, and then lint for it in CI.

Copy link
Contributor

@umanwizard umanwizard left a comment

Choose a reason for hiding this comment

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

yes, I love this

@benesch
Copy link
Member Author

benesch commented Aug 4, 2021

I propose that we solve this socially, by making it taboo to add #![allow(missing_docs)] unless you've gotten buy-in from the overall owner of that area of the codebase.

We could remove the four occurrences, and then lint for it in CI.

Yeah, the big sticking point there is https://github.com/MaterializeInc/materialize/blob/main/src/pgrepr/src/oid.rs. I feel like enabling missing_docs there would be more pain than it's worth? I could be convinced, though, by the simplicity of the resulting lint. I sent #7693 to remove one of the other allow(missing_docs) lints. That just leaves two other instances which should be fixed by adding docs.

@benesch benesch merged commit 4d2f427 into MaterializeInc:main Aug 4, 2021
@benesch benesch deleted the warn-everywhere branch August 4, 2021 20:06
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

4 participants