-
Notifications
You must be signed in to change notification settings - Fork 11
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
Remove deny warnings #126
Remove deny warnings #126
Conversation
@@ -46,7 +46,7 @@ release: tools | |||
|
|||
static: tools docs | |||
cargo fmt -- --check | |||
cargo clippy $(FEATURES) | |||
cargo clippy $(FEATURES) -- -Dwarnings |
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.
Can you add this to the build --all-targets
in the test target too? I'm not sure clippy checks everything like benchmarks, etc, right?
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.
It doesn't recognize the option, and it seems like this is still an open request for Cargo#8424. Yes this might miss some things but I'm not sure what the right solution would be. Maybe we would have to put #![deny(warnings)]
in test code?
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.
This wouldn't be making anything worse though right? Because test/benchmark targets would be built separately and not respect the directive in the lib/bin anyway? So we're making CI more strict and it will definitely check the code we removed the directive from. I'm tempted to leave figuring out erroring on lints for benchmarks as a problem to solve
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.
One request, otherwise, looks good.
Summary
As discussed, remove deny warnings and rely on CI to catch warnings. See recommendations from clippy docs
I've also bumped the versions in the same PR so I can release this once the PR is in
TODO: