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

Use Option::filter instead of open-coding it #77882

Closed
wants to merge 1 commit into from

Conversation

LingMan
Copy link
Contributor

@LingMan LingMan commented Oct 13, 2020

No description provided.

@rust-highfive
Copy link
Collaborator

r? @estebank

(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 Oct 13, 2020
Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

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

Looks good to me. @LingMan This is the way to go if you want to do these style changes, I think keeping it small have a higher chance of being accepted.

@bors
Copy link
Contributor

bors commented Oct 14, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@Mark-Simulacrum
Copy link
Member

I am not a maintainer of this particular code section, but the new version looks significantly less readable to me.

cc @matklad @petrochenkov as well

@matklad
Copy link
Member

matklad commented Oct 14, 2020

Yeah, I also don't think this is an improvement. Although the high-level semantic here is indeed that of Option::filter, closures do make code noisier and harder to understand. I don't see any clear cut ways to improve the code (? would make the shape awkward, and replacing unwrap_or_else with simpler unwrap_or would survive only until someone runs clippy again), so I think it makes sense to close the PR.

Thanks for the PR anyway, @LingMan, it's useful to see two versions side by side to better understand which one is better!

@matklad matklad closed this Oct 14, 2020
@LingMan
Copy link
Contributor Author

LingMan commented Oct 14, 2020

Huh, interesting. I find it more readable because there's less brain power spent on deciphering the high-level semantic and more focus on what's relevant.
Your code, your choice. Thanks for considering it!

@LingMan LingMan deleted the ast_pretty_filter branch October 14, 2020 21:59
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.

7 participants