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

Improvement for equatable_if_let #9368

Merged
merged 3 commits into from
Oct 23, 2022

Conversation

nahuakang
Copy link
Contributor

@nahuakang nahuakang commented Aug 23, 2022

fixes #9221

This PR makes sure that enums or structs not implementing PartialEq trait but still using the if let patterns can be linted to be rewritten with matches!.

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

  • Followed [lint naming conventions][lint_naming]
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed cargo dev update_lints
  • Added lint documentation
  • Run cargo dev fmt

changelog: Improve [equatable_if_let] with additional matches! suggestions.

@rust-highfive
Copy link

r? @flip1995

(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 23, 2022
@nahuakang nahuakang marked this pull request as ready for review August 23, 2022 18:17
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 the PR! This looks good to me.

I'll wait for @flip1995's review if there is any concern with merging this. Particularly, I'm not entirely sure if there's any scenario where we don't want to suggest matches!.

@dswij
Copy link
Member

dswij commented Aug 27, 2022

Just one comment, though, @nahuakang can you help to write the changelog field in the PR description

@nahuakang
Copy link
Contributor Author

@flip1995 Btw when you have time please take a look here :)

@dswij
Copy link
Member

dswij commented Sep 19, 2022

Actually @nahuakang, I found a discussion for this very issue #7777 - and it's unresolved yet.

Apologies for just finding this out after you made this PR 😟

@nahuakang
Copy link
Contributor Author

@dswij No problem :) Thanks for finding it out. So what's the status for either PR? Should I close mine in favor of the other or adjust something?

@dswij
Copy link
Member

dswij commented Sep 30, 2022

@dswij No problem :) Thanks for finding it out. So what's the status for either PR? Should I close mine in favor of the other or adjust something?

Seems like the ongoing discussion is not for the matches! case although the consensus there is somewhat on making matches! as a new lint rather than an improvement of equatable_if_let.

I will try to bring this up in Zulip.

@flip1995
Copy link
Member

The discussion in the other PR is not really affected by this change, so I'll merge it like this.

Wow, I'm really behind on reviews (this is already from 2 months ago). Sorry about that.

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 23, 2022

📌 Commit 4eaadd6 has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 23, 2022

⌛ Testing commit 4eaadd6 with merge 628a79d...

@bors
Copy link
Collaborator

bors commented Oct 23, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 628a79d to master...

@bors bors merged commit 628a79d into rust-lang:master Oct 23, 2022
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.

lint matchable if let for pattern
5 participants