-
Notifications
You must be signed in to change notification settings - Fork 35
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
adding clippy for windows #1027
Conversation
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.
Assuming habitat-sh/habitat#6430 works out, I think we should make these small tweaks here. Aside from that, it looks great!
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.
Nothing strictly required, but I think we can make things a bit more readable with some tweaks
b3df794
to
5fd805a
Compare
test/allowed_lints.txt
Outdated
clippy::module_inception | ||
clippy::new_ret_no_self | ||
clippy::new_without_default | ||
clippy::redundant_closure |
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'm still not comfortable with moving this to allowed. I know rust-lang/rust-clippy#3071 exists, but I've yet to see it flag something in our code that can't be fixed, even if the automatic suggestion isn't a correct fix. And for anything we decide we really prefer with an explicit closure, we can #[allow(…)]
it.
I'm ok with moving redundant_closure to lints_to_fix
, but I still find this lint valuable, and I imagine issue 3071 will be fixed at some point.
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.
Moving to lints_to_fix would be ok as well. Given that the clippy default is warn not deny, and there are known issues, it's also reasonable to just allow it and move on.
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.
Signed-off-by: mwrock <[email protected]>
Resolves habitat-sh/habitat#6213
Signed-off-by: mwrock [email protected]