-
Notifications
You must be signed in to change notification settings - Fork 13
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
Create lint groups #98
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
After testing (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Only some nits.
/// These are designed for scenarios where you want to increase the consistency of your code-base | ||
/// and reject certain patterns. They should not all be enabled at once, but instead specific lints | ||
/// should be individually enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the obvious question is, "if they shouldn't all be enabled at once, why are they in a group"? Or are we saying, this group is for categorising but not for actual use 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want every lint to be within a group so that it's clear what its general purpose is. These lint groups are just as important for documentation as they are for bulk-toggling.
So yes, this group is a category that's never intended to be named directly. (Clippy has the same things!)
Co-authored-by: Rich Churcher <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow failed to approve first time around.
Oh, I'm not on the repo so doesn't matter! 😁 |
Looks like I forgot to register the lint groups introduced in #98. Whoops! While I'm at it, I also changed the visibility of some items so that less is public that before.
Closes #90.
This PR implements lint groups! These, just like Clippy's groups, allow enabling and disable multiple lints at a time based on their categories. I, stealing Clippy's groups, have added:
while
you wrote there could be afor
loop! (This category checks for overly-complex code with good alternatives.)std::thread::sleep()
, that's your own fault.)Reflect
.bevy_lint
hasn't been released yet!As part of this PR, I've changed a few things about how lints are defined.
declare_bevy_lint!
macro. This is very similar to thedeclare_tool_lint!
that we've used before.BevyLint
struct: contains theLint
struct and a reference to the lint group this is a member of.LintGroup
struct: represents the name and default level (deny, warn, allow) of a lint.Old comment about an error I fixed
As of yet I'm unable to actually deny / allow a lint group. I've tried:
But I'm getting:
main_return_without_appexit
should have been denied as an error, but it was a warning instead. This requires a bit more testing before it can be merged.