Skip to content

Refactor Option::filter method#45933

Merged
bors merged 1 commit intorust-lang:masterfrom
shanavas786:refactor-filter
Nov 13, 2017
Merged

Refactor Option::filter method#45933
bors merged 1 commit intorust-lang:masterfrom
shanavas786:refactor-filter

Conversation

@shanavas786
Copy link
Contributor

@shanavas786 shanavas786 commented Nov 11, 2017

No description provided.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 11, 2017
sinkuu
sinkuu reviewed Nov 11, 2017
View reviewed changes
src/libcore/option.rs Outdated
Copy link
Contributor

@sinkuu sinkuu Nov 11, 2017

Choose a reason for hiding this comment

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

Nitpicking: ; is unnecessary here.

Copy link
Contributor Author

@shanavas786 shanavas786 Nov 11, 2017

Choose a reason for hiding this comment

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

Yeah, Updated. Thanks

@alexcrichton
Copy link
Member

alexcrichton commented Nov 12, 2017

@bors: r+

@bors
Copy link
Collaborator

bors commented Nov 12, 2017

📌 Commit abff092 has been approved by alexcrichton

@alexcrichton alexcrichton self-assigned this Nov 12, 2017
@kennytm kennytm 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 Nov 12, 2017
kennytm added a commit to kennytm/rust that referenced this pull request Nov 13, 2017
@kennytm
Rollup merge of rust-lang#45933 - shanavas786:refactor-filter, r=alex…
3604737 
…crichton

Refactor Option::filter method
@kennytm kennytm mentioned this pull request Nov 13, 2017
Merged
bors added a commit that referenced this pull request Nov 13, 2017
@bors
Auto merge of #45956 - kennytm:rollup, r=kennytm
e312c8a 
Rollup of 9 pull requests

- Successful merges: #45828, #45892, #45893, #45914, #45917, #45927, #45933, #45952, #45954
- Failed merges:
@bors bors merged commit abff092 into rust-lang:master Nov 13, 2017
@LukasKalbertodt
Copy link
Contributor

LukasKalbertodt commented Nov 15, 2017

What was the reason for this change? I don't really care that my code was changed, I just don't really understand why. I guess it's because it has fewer lines and only one None? Was this change discussed in IRC? I would really appreciate it, if PRs for such changes would have at least a short description. Otherwise the rest of the world has no clue what's going on.

@bstrie
Copy link
Contributor

bstrie commented Nov 15, 2017

@LukasKalbertodt I surmise it was just mildly more idiomatic to use if let to reduce the slight amount of repetition.

@Kerollmops
Copy link
Contributor

Kerollmops commented Nov 15, 2017 •
edited
Loading

Why not this so ?

match self {
    Some(x) if predicate(&x) => Some(x),
    _ => None
}

@sinkuu
Copy link
Contributor

sinkuu commented Nov 16, 2017

@Kerollmops That would be cetainly better, but currently impossible (produces an error E0008).

@Kerollmops
Copy link
Contributor

Kerollmops commented Nov 16, 2017

Ho ! you're right ! sorry ! I hope the NLL will change this kind of error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@sinkuu sinkuu sinkuu left review comments

@sugar700 sugar700 sugar700 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@alexcrichton alexcrichton

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.

9 participants

@shanavas786 @alexcrichton @bors @LukasKalbertodt @bstrie @Kerollmops @sinkuu @sugar700 @kennytm