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

Implement Option::contains and Result::contains #62356

Merged
merged 1 commit into from
Jul 8, 2019
Merged

Implement Option::contains and Result::contains #62356

merged 1 commit into from
Jul 8, 2019
+78 −0

Conversation

soc
Copy link
Contributor

@soc soc commented Jul 3, 2019

This increases consistency with other common data structures.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(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 Jul 3, 2019
@Centril Centril mentioned this pull request Jul 4, 2019
Closed
Centril
Centril reviewed Jul 4, 2019
View reviewed changes
src/libcore/option.rs Outdated Show resolved Hide resolved
src/libcore/result.rs Outdated Show resolved Hide resolved
src/libcore/result.rs Show resolved Hide resolved
Centril
Centril reviewed Jul 4, 2019
View reviewed changes
src/libcore/option.rs Show resolved Hide resolved
src/libcore/result.rs Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Jul 4, 2019

I filed a tracking issue for you in #62358.

@Centril
Copy link
Contributor

Centril commented Jul 4, 2019

This seems like quite a useful addition to me!

I know I've wanted this a bunch of times and opt.filter(|x| == y).is_some() doesn't read as well / is not as direct.

@cramertj
Copy link
Member

cramertj commented Jul 4, 2019

opt.filter(|x| == y).is_some() doesn't read as well / is not as direct.

FWIW, I've always done opt.map(|x| x == y).unwrap_or(false)

@Centril
Copy link
Contributor

Centril commented Jul 4, 2019

r? @Centril

@rust-highfive rust-highfive assigned Centril and unassigned alexcrichton Jul 4, 2019
@soc
Copy link
Contributor Author

soc commented Jul 4, 2019

@Centril Thanks! I'll squash the commits as soon as you are happy with the code.

@Centril
Copy link
Contributor

Centril commented Jul 4, 2019

@soc It looks good to me so go ahead and squash now :)

As for fn contains_err / fn err_contains that can be deferred to some future PR.

@Centril
Copy link
Contributor

Centril commented Jul 4, 2019

@soc Can you please apply @Mark-Simulacrum's suggestion in #62358 (comment)?

@soc
Copy link
Contributor Author

soc commented Jul 4, 2019

It looks good to me so go ahead and squash now :)

Will do!

As for fn contains_err / fn err_contains that can be deferred to some future PR.

Agreed.

I think there are also a few additional combinators that could be considered (like filter_...). Might make sense to get the bigger picture on operations that are not used all the time, but are painful to work around their absence.

@petrochenkov
Copy link
Contributor

I usually use my_option == Some(value) for this and it works most of the time.

For the cases where it doesn't work it would be much more useful to implement heterogeneous comparisons T: PartialEq<U> => Option<T>: PartialEq<Option<U>>, but that's blocked on type parameter fallback in type inference.

fbstj
fbstj reviewed Jul 4, 2019
View reviewed changes
src/libcore/result.rs Outdated Show resolved Hide resolved
@soc
Copy link
Contributor Author

soc commented Jul 4, 2019

@Centril Added contains_err and squashed. Good to go?

@Centril
Copy link
Contributor

Centril commented Jul 5, 2019 •
edited
Loading

@soc Seems you have some test failures (see https://dev.azure.com/rust-lang/rust/_build/results?buildId=2412). While fixing those, can you also address @fbstj's comment? -- Also, can you split contains_err into a separate feature gate while at it?

@Centril Centril 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 Jul 5, 2019
@soc
Copy link
Contributor Author

soc commented Jul 5, 2019

@Centril Will do, thanks!

@soc
Copy link
Contributor Author

soc commented Jul 6, 2019

@Centril Done!

@Centril
Copy link
Contributor

Centril commented Jul 6, 2019

Thanks! r=me rollup when green

@soc soc force-pushed the topic/contains branch 2 times, most recently from 084bfd2 to 4e37908 Compare July 7, 2019 12:18
@soc
Implement Option::contains, Result::contains and Result::contains_err
6f76da4 
This increases consistency with other common data structures.
@soc
Copy link
Contributor Author

soc commented Jul 7, 2019

@Centril Tests are green!

@Centril
Copy link
Contributor

Centril commented Jul 7, 2019

Thanks! @bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 7, 2019

📌 Commit 6f76da4 has been approved by Centril

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 7, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 8, 2019
@Centril
Rollup merge of rust-lang#62356 - soc:topic/contains, r=Centril
ddf630d 
Implement Option::contains and Result::contains

This increases consistency with other common data structures.
@Centril Centril mentioned this pull request Jul 8, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jul 8, 2019
@Centril
Rollup merge of rust-lang#62356 - soc:topic/contains, r=Centril
6da3098 
Implement Option::contains and Result::contains

This increases consistency with other common data structures.
@Centril Centril mentioned this pull request Jul 8, 2019
Merged
bors added a commit that referenced this pull request Jul 8, 2019
@bors
Auto merge of #62485 - Centril:rollup-gg3it1u, r=Centril
db592f4 
Rollup of 5 pull requests

Successful merges:

 - #62356 (Implement Option::contains and Result::contains)
 - #62462 (Document `while` keyword)
 - #62472 (Normalize use of backticks in compiler messages p2)
 - #62477 (Re-add bootstrap attribute to libunwind for llvm-libunwind feature)
 - #62478 (normalize use of backticks for compiler messages in librustc_codegen)

Failed merges:

r? @ghost
@bors bors merged commit 6f76da4 into rust-lang:master Jul 8, 2019
@soc soc deleted the topic/contains branch July 8, 2019 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centril Centril Centril left review comments

@fbstj fbstj fbstj left review comments

Assignees

@Centril Centril

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
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@soc @rust-highfive @Centril @cramertj @petrochenkov @bors @fbstj @alexcrichton