Fix 8128 by surechen · Pull Request #8133 · rust-lang/rust-clippy · GitHub
source link: https://github.com/rust-lang/rust-clippy/pull/8133
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Fixes #8128
changelog: Fix error suggestion of skip(..).next() for immutable variable.
r? @flip1995
(rust-highfive has picked a reviewer for you, use r? to override)
The tests are failing, because rustfix can only apply suggestions produces by span_lint_and_sugg/span_suggestion. So you probably have to create a separate test file, that doesn't have the // run rustfix comment at the top.
clippy_lints/src/methods/iter_skip_next.rs
Outdated Show resolved
Hi, could you help me see this? Is it consistent with your modification suggestion? Thanks.
r? @xFrednet
Hi, could you help me see this? Is it consistent with your modification suggestion?
Sure, this is what I had in mind. I again looked into a way to check this differently, but I don't see a viable solution to get the information from the type of the self argument of skip. And I think this is fine, since @flip1995 didn't mention another solution in his first review.
I have two suggestions how the output could be a bit more readable, but otherwise this looks good to me. Thank you for the work 
Hi, could you help me see this? Is it consistent with your modification suggestion?
Sure, this is what I had in mind. I again looked into a way to check this differently, but I don't see a viable solution to get the information from the type of the
selfargument ofskip. And I think this is fine, since@flip1995didn't mention another solution in his first review.I have two suggestions how the output could be a bit more readable, but otherwise this looks good to me. Thank you for the work
Done. Thanks.
Looks good to me! 
Could you please squash your commits before the merge? If you want to keep the messages, you can squash the fix #8128 commits into one and all review commits into another one.
A related note: In Rust we usually avoid mentioning the issue in commit messages as that creates a new link in the issue with every rebase. I do see the advantage of mentioning it and use that feature in other projects as well. I think the idea is more to use a descriptive commit message that can be understood without opening an issue. We can also merge commits with such messages, and I'm happy to do so if you want to keep the messages. Just something I wanted to mention, this also depends on your personal style
Looks good to me!
Could you please squash your commits before the merge? If you want to keep the messages, you can squash the
fix #8128commits into one and all review commits into another one.A related note: In Rust we usually avoid mentioning the issue in commit messages as that creates a new link in the issue with every rebase. I do see the advantage of mentioning it and use that feature in other projects as well. I think the idea is more to use a descriptive commit message that can be understood without opening an issue. We can also merge commits with such messages, and I'm happy to do so if you want to keep the messages. Just something I wanted to mention, this also depends on your personal style
Done. Thank you very much.
LGTM, thank you for the update and quick replies!
@bors r+
Commit 4ffd660 has been approved by xFrednet
Testing commit Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK