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

fix bug on mutable ref #7533

Merged
merged 1 commit into from
Aug 7, 2021
Merged

Conversation

Valentine-Mario
Copy link
Contributor

@Valentine-Mario Valentine-Mario commented Aug 5, 2021

fixes: #7524

This PR is related to issue #7524
changelog: [extend_with_drain] Improve code suggestion for mutable and immutable references

@rust-highfive
Copy link

r? @llogiq

(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 Aug 5, 2021
@Valentine-Mario
Copy link
Contributor Author

@xFrednet

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Requires cargo dev bless after the requested changes

clippy_lints/src/methods/extend_with_drain.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/extend_with_drain.rs Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member

xFrednet commented Aug 5, 2021

Thank you for pinging me, glad to see that you got it working 🙃 .

Could you update the PR description to include a changelog entry for this update and add a line like fixes #7524 (that will close the issue automatically with the merge)?

One more thing, Rust has a no merge policy that Clippy also tries to follow. Could you rebase your commit on master and remove the merge commit from this branch? 🙃

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.

The rest looks good.

Thank you for addressing my comment about the description and commits. For the next time, it would be nice if you could give a bit more context in the commit message what actually got fixed 🙃.

I'm okay with merging this when my comments have been updated 👍

clippy_lints/src/methods/extend_with_drain.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/extend_with_drain.rs Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member

xFrednet commented Aug 5, 2021

Awesome, the code looks good to me!

As a last step. Could you add a change log entry to your PR description like this:

changelog:  [`extend_with_drain`] <describe change here>

You can just describe the change like: "Improved suggestion for mutable references" or something similar. You can ping me once you're done to merge this 🙃

@Valentine-Mario
Copy link
Contributor Author

@xFrednet

@xFrednet
Copy link
Member

xFrednet commented Aug 5, 2021

Everything looks good to me, thank you for the fix 👍.

@bors r+


Sorry for stealing the PR review from you @llogiq 😅

@bors
Copy link
Contributor

bors commented Aug 5, 2021

📌 Commit 8a4ffb8 has been approved by xFrednet

@flip1995
Copy link
Member

flip1995 commented Aug 6, 2021

@bors retry

@flip1995
Copy link
Member

flip1995 commented Aug 6, 2021

bors seems to be broken right now? https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/bors.20doesn't.20trigger.20auto.20CI.20on.20libc/near/248506490

@bors
Copy link
Contributor

bors commented Aug 7, 2021

⌛ Testing commit 8a4ffb8 with merge a0e71e5...

@bors
Copy link
Contributor

bors commented Aug 7, 2021

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

@bors bors merged commit a0e71e5 into rust-lang:master Aug 7, 2021
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.

bad fix suggested for non-mut ref to a mut value
6 participants