-
Notifications
You must be signed in to change notification settings - Fork 464
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
Some clippy fixes #879
Some clippy fixes #879
Conversation
Perhaps it'd make sense to add clippy to CI? |
Yes it does, if you are fixing the clippy then it makes sense to also add it to the CI. |
I'll rebase this, see if I had fixed everything, and if so, add it to CI. |
39637ac
to
0ec230e
Compare
There are still a number of clippy warnings, so that's why I hadn't enabled it in CI yet. |
Can you add a test that only fails on clippy errors, then? Clippy doesn't default-error for a lot, but on the things it does error for, they're pretty bad. |
These are part of Windows FFI glue code and the Windows types use names in uppercase.
0ec230e
to
244805a
Compare
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.
Hi, I agree with @workingjubilee that adding clippy run that failed on clippy errors in CI is required for this PR to merge.
If you are willing to add that, I'm willing to approve and merge it.
I will work on it! |
f7b780f
to
fc198f1
Compare
For better or worse, the So they have to be fixed to add the new CI job. @NobodyXu This should be re-ready for review. There's still a bunch of clippy warnings ... |
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.
LGTM, having warning is ok
No description provided.