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

suggests is_some_and over map().unwrap #11030

Merged
merged 8 commits into from
Jun 29, 2023
Merged

Conversation

darklyspaced
Copy link
Contributor

@darklyspaced darklyspaced commented Jun 26, 2023

changelog: Enhancement: [option_map_unwrap_or] now considers the [msrv] config when creating the suggestion.

  • modified option_map_unwrap_or lint to recognise when an Option<T> is mapped to an Option<bool> with false being used when None is detected; suggests the use of is_some_and instead
  • msrv is set to 1.70.0 for this lint; when is_some_and was stabilised

fixes #9125

@rustbot
Copy link
Collaborator

rustbot commented Jun 26, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 26, 2023
@darklyspaced darklyspaced reopened this Jun 26, 2023
@darklyspaced
Copy link
Contributor Author

darklyspaced commented Jun 26, 2023

apologies for the slightly questionably git history.

dogfood was failing because check had too many arguments after adding support for msrv discrimination on the lint. i wasn't exactly sure on the most idiomatic way to fix this, so i just wrapped two similar arguments into a tuple which i then destructed inside the function.

please let me know if there is a better way to get around the too many arguments warning.

@xFrednet
Copy link
Member

@Centri3 Do you want to take over the review for this PR? :)

@Centri3
Copy link
Member

Centri3 commented Jun 26, 2023

Sure, I'll take a look in a bit :D

Copy link
Member

@Centri3 Centri3 left a comment

Choose a reason for hiding this comment

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

Looks pretty good overall, I've left some small comments. Thanks! :D

clippy_utils/src/msrvs.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/option_map_unwrap_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/option_map_unwrap_or.rs Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member

xFrednet commented Jun 28, 2023

This version looks good to me, just a tiny nit, then it's ready to be merged. Thank you for the PR, I hope you had fun working on Clippy!

@Centri3 Thank you for the review. I leave the final r+ to you.

@bors delegate=Centri3

@xFrednet

This comment was marked as off-topic.

@xFrednet

This comment was marked as off-topic.

@bors

This comment was marked as off-topic.

@bors
Copy link
Contributor

bors commented Jun 28, 2023

✌️ @Centri3, you can now approve this pull request!

If @xFrednet told you to "r=me" after making some further change, please make that change, then do @bors r=@xFrednet

Copy link
Member

@Centri3 Centri3 left a comment

Choose a reason for hiding this comment

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

This looks good, thank you :)

@bors r=Centri3,xFrednet

@Centri3
Copy link
Member

Centri3 commented Jun 29, 2023

Well bors is still asleep. But, @darklyspaced, I do believe you need to run cargo collect-metadata. I looked again and saw no changes to lint configuration, unfortunately dogfood itself doesn't check it, only CI does when testing an approved PR (which should definitely be changed btw, as it's really annoying)

Once that's been ran, I'll try reapproving again :D (also if there's no changes ^^)

@Centri3
Copy link
Member

Centri3 commented Jun 29, 2023

Thank you!

@bors r=Centri3,xFrednet

@bors
Copy link
Contributor

bors commented Jun 29, 2023

📌 Commit bb42b18 has been approved by Centri3,xFrednet

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 29, 2023

⌛ Testing commit bb42b18 with merge 9020937...

@xFrednet
Copy link
Member

Well bors is still asleep

@bors comments sadly don't work in the review comments. That's why you always see everyone first approving the PR and then adding a text comment with the LGTM and r+ :)

cc: @blyxyas this might also be interesting for you :)

@bors
Copy link
Contributor

bors commented Jun 29, 2023

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

@bors bors merged commit 9020937 into rust-lang:master Jun 29, 2023
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.

Use is_some_and when appropriate
5 participants