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

More practical examples for Option::and_then & Result::and_then #93851

Merged
merged 8 commits into from
Feb 13, 2022
Merged

More practical examples for Option::and_then & Result::and_then #93851

merged 8 commits into from
Feb 13, 2022

Conversation

cyqsimon
Copy link
Contributor

@cyqsimon cyqsimon commented Feb 10, 2022 •
edited
Loading

To be blatantly honest, I think the current example given for Option::and_then is objectively terrible. (No offence to whoever wrote them initially.)

fn sq(x: u32) -> Option<u32> { Some(x * x) }
fn nope(_: u32) -> Option<u32> { None }

assert_eq!(Some(2).and_then(sq).and_then(sq), Some(16));
assert_eq!(Some(2).and_then(sq).and_then(nope), None);
assert_eq!(Some(2).and_then(nope).and_then(sq), None);
assert_eq!(None.and_then(sq).and_then(sq), None);

Current example:

  • does not demonstrate that and_then converts Option<T> to Option<U>
  • is far removed from any realistic code
  • generally just causes more confusion than it helps

So I replaced them with two blocks:

  • the first one shows basic usage (including the type conversion)
  • the second one shows an example of typical usage

Same thing with Result::and_then.

Hopefully this helps with clarity.

@rust-highfive
Copy link
Collaborator

r? @scottmcm

(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 Feb 10, 2022
@scottmcm
Copy link
Member

Thanks for the PR! These look good to me.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 10, 2022

📌 Commit 73a5f01 has been approved by scottmcm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2022
@cyqsimon cyqsimon closed this Feb 10, 2022
@cyqsimon cyqsimon deleted the option-examples branch February 10, 2022 10:00
@cyqsimon cyqsimon restored the option-examples branch February 10, 2022 10:01
@cyqsimon cyqsimon reopened this Feb 10, 2022
@cyqsimon
Copy link
Contributor Author

Oops. I renamed the branch on GitHub expecting this PR to be unaffected. Sry for any inconvenience.

@cyqsimon
Copy link
Contributor Author

I also updated examples for Result::and_then, and thought I'd squeeze that into this PR as well. Well turns out I was somewhat too slow. Hopefully I haven't messed up the workflow too bad.

@cyqsimon cyqsimon changed the title More practical examples for Option::and_then More practical examples for Option::and_then & Result::and_then Feb 10, 2022
@scottmcm
Copy link
Member

@rustbot author

I'm going to mark this as author, so you can continue making additions.

Once you're at the final version, use @rustbot ready to mark it as review-ready.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 11, 2022
@scottmcm scottmcm removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 11, 2022
@cyqsimon
Copy link
Contributor Author

My apologies, I really should have made it a draft PR before everything is ready. I will do better next time. Sorry for the disruptions I caused.

I'm happy with the current state of this PR and will refrain from making further edits. Thanks.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 11, 2022
scottmcm
scottmcm requested changes Feb 12, 2022
View reviewed changes
Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

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

Just a couple of thoughts on the Result examples.

I don't think they're MUSTs, so if you'd rather not make changes then comment and re-ready the PR.

library/core/src/result.rs Outdated Show resolved Hide resolved
library/core/src/result.rs Show resolved Hide resolved
@scottmcm scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 12, 2022
@cyqsimon
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 12, 2022
@scottmcm
Copy link
Member

Thanks for the updates!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 12, 2022

📌 Commit f6f93fd has been approved by scottmcm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 12, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 13, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2022
@bors
Auto merge of rust-lang#93956 - matthiaskrgr:rollup-zfk35hb, r=matthi…
9a60099 
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#89926 (make `Instant::{duration_since, elapsed, sub}` saturating and remove workarounds)
 - rust-lang#90532 (More informative error message for E0015)
 - rust-lang#93810 (Improve chalk integration)
 - rust-lang#93851 (More practical examples for `Option::and_then` & `Result::and_then`)
 - rust-lang#93885 (bootstrap.py: Suggest disabling download-ci-llvm option if url fails to download)
 - rust-lang#93886 (Stabilise inherent_ascii_escape (FCP in rust-lang#77174))
 - rust-lang#93930 (add link to format_args! when mention it in docs)
 - rust-lang#93936 (Couple of driver cleanups)
 - rust-lang#93944 (Don't relabel to a team if there is already a team label)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 783b56b into rust-lang:master Feb 13, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 13, 2022
@cyqsimon cyqsimon deleted the option-examples branch February 14, 2022 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottmcm scottmcm scottmcm requested changes

Assignees

@scottmcm scottmcm

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
5 participants
@cyqsimon @rust-highfive @scottmcm @bors @rustbot