Skip to content

Refactor Option::filter method #45933

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
Merged
merged 1 commit into from
Nov 13, 2017
Merged

Refactor Option::filter method #45933

merged 1 commit into from
Nov 13, 2017

Conversation

shanavas786
Copy link
Contributor

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
}
None => None,
}
};
Copy link
Contributor

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

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

@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

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

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

@LunaBorowska LunaBorowska LunaBorowska 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 @LunaBorowska @kennytm