-
Notifications
You must be signed in to change notification settings - Fork 323
style: address some clippy warnings #813
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
base: master
Are you sure you want to change the base?
Conversation
martin-g
left a comment
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!
| return; | ||
| } else if self.capacity() == 0 { | ||
| } | ||
| if self.capacity() == 0 { |
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.
Is this also suggested by Clippy ?
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.
Is this also suggested by Clippy ?
It is
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 don't care about what clippy thinks. This makes the function more confusing to read, and it's probably one of the pedantic lints anyway.
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 is a pedantic lint and i'd be happy to revert in this case.
i've not enforced pedantic lints in #814
This would be nice! |
I can take a look at that in a future PR |
|
see #814 |
| assert!(!buf.try_reclaim(6)); | ||
| buf.reserve(6); | ||
| assert_eq!(true, buf.try_reclaim(6)); | ||
| assert!(buf.try_reclaim(6)); | ||
| let cap = buf.capacity(); | ||
| assert!(cap >= 6); | ||
| assert_eq!(false, buf.try_reclaim(cap + 1)); | ||
| assert!(!buf.try_reclaim(cap + 1)); |
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.
These are not helpful. It's perfectly readable as-is.
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.
these are default clippy lints, not pedantic lints, which means if you have clippy enabled in your IDE you get spammed with these warnings. This 'noise' can hide more impactful lints.
i don't have any strong feelings about this lint in particular, but i think it's useful for contributors to both-
- use clippy
- reduce the noise generated by clippy so the results are meaningful
there are two ways to reduce that noise. Either you can address the lints, or you can explicitly suppress them. I don't care which, but i think the status quo is an issue.
addresses some clippy warnings (including some pedantic ones)
i see there's a commented out clippy linting job- does that mean there's an ambition to sort out the warnings and enable linting?