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

add or_then_unwrap #8561

Merged
merged 17 commits into from
Mar 21, 2022
Merged

add or_then_unwrap #8561

merged 17 commits into from
Mar 21, 2022

Conversation

FoseFx
Copy link
Contributor

@FoseFx FoseFx commented Mar 17, 2022

Closes #8557

changelog: New lint [or_then_unwrap]

@rust-highfive
Copy link

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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 17, 2022
tests/ui/use_unwrap_or.rs Outdated Show resolved Hide resolved
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.

Welcome to the project @FoseFx 👋. I like the lint idea and the direction of your implementation is also looking good. I've added some comments with explanations and suggestions. Your welcome to reach out if anything is unclear or if you just would like some more background information 🙃

clippy_lints/src/use_unwrap_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/use_unwrap_or.rs Outdated Show resolved Hide resolved
tests/ui/use_unwrap_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/use_unwrap_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/use_unwrap_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/use_unwrap_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/use_unwrap_or.rs Outdated Show resolved Hide resolved
@FoseFx FoseFx changed the title add use_unwrap_or add or_then_unwrap Mar 18, 2022
@FoseFx FoseFx requested a review from xFrednet March 20, 2022 10:19
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.

This version is already looking good. It's also nice to see that you were able to move this into the methods module. The suggestion creation is also implemented correctly. I've added four small comments, and then it can be merged. Thank you very much for the work you've done so far 🙃

clippy_lints/src/methods/or_then_unwrap.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Show resolved Hide resolved
clippy_lints/src/methods/or_then_unwrap.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/or_then_unwrap.rs Outdated Show resolved Hide resolved
@FoseFx
Copy link
Contributor Author

FoseFx commented Mar 20, 2022

Alright, tests are 💚. Tysm for your help!

@xFrednet
Copy link
Member

Looks good to me, thank you for your contribution. I hope you also had fun while working on this 🙃

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2022

📌 Commit 765cce1 has been approved by xFrednet

@bors
Copy link
Contributor

bors commented Mar 21, 2022

⌛ Testing commit 765cce1 with merge 4a07662...

@bors
Copy link
Contributor

bors commented Mar 21, 2022

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

@bors bors merged commit 4a07662 into rust-lang:master Mar 21, 2022
@FoseFx FoseFx deleted the use_unwrap_or branch March 21, 2022 20:25
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.

Favor unwrap_or over .or(Some(value)).unwrap()
4 participants