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.

Sign up for GitHub

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

Jump to bottom

new lint: option_as_ref_cloned #12051

Merged
merged 1 commit into from
Jan 5, 2024
Merged

new lint: option_as_ref_cloned #12051

merged 1 commit into from
Jan 5, 2024

Conversation

y21
Copy link
Member

@y21 y21 commented Dec 30, 2023

Closes #12009

Adds a new lint that looks for .as_ref().cloned() on Options. That's the same as just .clone()-ing the option directly.

changelog: new lint: [option_as_ref_cloned]

@rustbot
Copy link
Collaborator

rustbot commented Dec 30, 2023

r? @dswij

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 30, 2023
@y21
Copy link
Member Author

y21 commented Dec 30, 2023

Extended it to also cover .as_mut().cloned() since the same argument can be made for that.

@bors
Copy link
Contributor

bors commented Dec 30, 2023

☔ The latest upstream changes (presumably #12054) made this pull request unmergeable. Please resolve the merge conflicts.

dswij
dswij approved these changes Jan 1, 2024
View reviewed changes
Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

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

Thanks for this! Looks pretty straightforward.

Can you help to rebase this to master? I think it's good to merge afterward.

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
@y21 y21 force-pushed the option_as_ref_cloned branch 2 times, most recently from f88b899 to 692ec68 Compare January 1, 2024 15:24
@dswij
Copy link
Member

dswij commented Jan 3, 2024

Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 3, 2024

📌 Commit 692ec68 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 3, 2024

⌛ Testing commit 692ec68 with merge dc2f975...

bors added a commit that referenced this pull request Jan 3, 2024
@bors
Auto merge of #12051 - y21:option_as_ref_cloned, r=dswij
dc2f975 
new lint: `option_as_ref_cloned`

Closes #12009

Adds a new lint that looks for `.as_ref().cloned()` on `Option`s. That's the same as just `.clone()`-ing the option directly.

changelog: new lint: [`option_as_ref_cloned`]
@bors
Copy link
Contributor

bors commented Jan 3, 2024

💔 Test failed - checks-action_dev_test

@dswij
Copy link
Member

dswij commented Jan 3, 2024

@y21 seems like you need to run cargo dev update_lints

@y21
Copy link
Member Author

y21 commented Jan 3, 2024

That's odd... That command doesn't change anything and cargo dev update_lints --check passes locally. I'm going to try rebasing and squashing the commits. Maybe it tried checking the first commit and there's something wrong with that? 🤔

@y21 y21 force-pushed the option_as_ref_cloned branch from 692ec68 to 9837a03 Compare January 3, 2024 18:34
@y21 y21 force-pushed the option_as_ref_cloned branch from 9837a03 to 5960107 Compare January 3, 2024 18:41
y21
y21 commented Jan 3, 2024
View reviewed changes
README.md
@@ -5,7 +5,7 @@

A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.

[There are over 650 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
[There are over 700 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
Copy link
Member Author

Choose a reason for hiding this comment

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

🎉

Looks like this is what caused CI to fail, it wanted this updated

Copy link
Member

@dswij dswij Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm quite surprised that we have this check.

But, did we surpass the 700 milestone with this? 🎉

@dswij
Copy link
Member

dswij commented Jan 5, 2024

Let's try again

@bors r+

@bors
Copy link
Contributor

bors commented Jan 5, 2024

📌 Commit 5960107 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 5, 2024

⌛ Testing commit 5960107 with merge ee8bfb7...

@bors
Copy link
Contributor

bors commented Jan 5, 2024

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

@bors bors merged commit ee8bfb7 into rust-lang:master Jan 5, 2024
8 checks passed
@psychon
Copy link

psychon commented Jan 5, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dswij dswij dswij approved these changes

Assignees

@dswij dswij

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Suggest to replace Option::as_ref().cloned() with Option::replace()
5 participants
@y21 @rustbot @bors @dswij @psychon