-
-
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
Add small refactorings #1847
Add small refactorings #1847
Conversation
Thanks for taking the time to make a contribution! May I ask if you could please split the change up into smaller commits? The smaller the better, essentially. If there turns out to be a regression, smaller commits will give higher resolution for both git bisect and git revert. They also make code review easier. I'm also curious if you used clippy to find these, or if you manually found them, if you don't mind me asking? |
Hi, thanks for having me! 😄 I'll split the change in a bit.
I scanned the code for common idioms manually, e.g. |
c57d1cb
to
62fd8f6
Compare
I think it looks nice in general! Thanks! Just had two comments on the last commit. (Please keep doing force-push so I can keep the commits when merging) |
62fd8f6
to
bf334c6
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.
This looks good to me now. Thanks!
I'll give other maintainers a chance to take a look before I merge.
Thanks for taking a look, Keith. Let's merge this now to not delay #951. |
Summary:
.iter()
/.into_iter()
callsif-else
blocks that can be easily short circuited.as_...()
calls