-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix some clippy warnings #1369
fix some clippy warnings #1369
Conversation
It looks like the
|
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 like most of these changes, but I'm not a fan of using matches!
everywhere. It has its uses, but I find it makes it harder to immediately understand what's happening. It's also not something I would consider worth bumping the minimum version for.
I've seen other projects disable that Clippy rule, and I would be in favor of doing it for bat. @sharkdp @keith-hall, your thoughts?
Ok, so both eth-p and sharkdp seems to agree they want to go for option 2:
If you confirm that I will do it :) |
I'd rather not add any clippy-annotations. If we can keep backwards compatibility for now, that would be great. |
Why? If we add them we can add clippy to the CI. |
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.
Thank you!
I don't want to force contributors to fix clippy suggestions in order to get a successful CI build. |
No description provided.