Skip to content
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

Cleanup some clippy lints #454

Merged
merged 10 commits into from Sep 4, 2020
Merged

Conversation

phimuemue
Copy link
Member

I cleaned up some things I got from clippy. I only changed what I'd consider straightforward.

I think it would eventually be beneficial to resolve the remaining lints, so that it is easier to identify real problems spotted by clippy. Do we have a policy regarding this?

Off-topic: I would have merged this right away, but I did not want to introduce conflicts with existing PRs. Is there a way to check if my commits would introduce conflicts?

@jswrenn
Copy link
Member

jswrenn commented Jun 26, 2020

I think it would eventually be beneficial to resolve the remaining lints, so that it is easier to identify real problems spotted by clippy. Do we have a policy regarding this?

Go for it!

Off-topic: I would have merged this right away, but I did not want to introduce conflicts with existing PRs. Is there a way to check if my commits would introduce conflicts?

I don't know of any way to quickly judge whether a PR introduces merge conflicts with other PRs. Before merging big PRs, I usually scan through the list of other PRs and see if there's any low-hanging fruit ready to be merged.

I'm currently on my honeymoon and COVID's made my work weird (I do education research), so I'm short on time. If you have the desire and time to knock down the list of pending PRs, feel free to go for it!

@phimuemue
Copy link
Member Author

I'm currently on my honeymoon

Hey, congratulations! Nice to hear this - I hope you're having a great time.

and COVID's made my work weird (I do education research), so I'm short on time.

Wish you all the best for your work.

@jplatte
Copy link
Contributor

jplatte commented Aug 26, 2020

Bump.

@phimuemue
Copy link
Member Author

phimuemue commented Sep 3, 2020

Bump.

@jplatte Have you thoroughly reviewed this? (I'd prefer a second pair of eyes on this, in particular the match bool -> if/else.)

If yes, and if @jswrenn has no objections, I'd merge this.

@jplatte
Copy link
Contributor

jplatte commented Sep 3, 2020

Have you thoroughly reviewed this?

I have now :)

The bool matching change looks good; there's one tiny thing I've noticed but I think this can be merged.

src/adaptors/mod.rs Outdated Show resolved Hide resolved
@jswrenn
Copy link
Member

jswrenn commented Sep 4, 2020

This looks good to me. Ready to merge, @phimuemue?

@phimuemue
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 4, 2020

Build succeeded:

@bors bors bot merged commit c48da7b into rust-itertools:master Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants