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

Change while_let_on_iterator suggestion to use by_ref() #7690

Merged
merged 1 commit into from
Sep 19, 2021

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Sep 19, 2021

It came up in the discussion #7659 that suggesting iter.by_ref() is a clearer suggestion than &mut iter. I personally think they're equivalent, but if by_ref() is clearer to people then that should be the suggestion.

changelog: Change while_let_on_iterator suggestion when using &mut to use by_ref()

@rust-highfive
Copy link

r? @xFrednet

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 19, 2021
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR.

I can see the reasoning why by_ref() might be preferred, even if they are equivalent. Using by_ref() looks a bit cleaner IMO, and it's also easier to find documentation for it. The std docs contains an example which illustrates the usage of by_ref() quite well. 🙃

One related question: It seems like this resolves the discussion from #7659 should we close the issue after this got merged? (I'll r+ this PR now, as it looks good to me. We can close the issue manually afterwards)

@xFrednet
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 19, 2021

📌 Commit ee6a6b5 has been approved by xFrednet

@bors
Copy link
Contributor

bors commented Sep 19, 2021

⌛ Testing commit ee6a6b5 with merge 871ad80...

@bors
Copy link
Contributor

bors commented Sep 19, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 871ad80 to master...

@bors bors merged commit 871ad80 into rust-lang:master Sep 19, 2021
@Jarcho
Copy link
Contributor Author

Jarcho commented Sep 20, 2021

There's still the question of whether the lint should be split between the two cases (whether by_ref() is needed or not).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants