Skip to content

Conversation

clarfonthey
Copy link
Contributor

This adds an expect_err method to Result. Considering how unwrap_err already exists, this seems to make sense. Inconsistency noted in Manishearth/rust-clippy#1435.

@rust-highfive
Copy link
Contributor

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@apasel422 apasel422 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 11, 2017
@aturon
Copy link
Contributor

aturon commented Jan 11, 2017

cc @rust-lang/libs

This fits conventions and makes sense to me! Before we land, we'll need to open a tracking issue and tag the stability attribute with it.

@alexcrichton
Copy link
Member

I like it!

///
/// Basic usage:
///
/// ```{.should_panic}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this syntax?

Copy link
Member

Choose a reason for hiding this comment

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

It's archaic but equivalent to just should_panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH I've wondered why you use this instead of just #![should_panic]. I just copied it from expect's docs.

@clarfonthey
Copy link
Contributor Author

If the general consensus is to merge, I can create a tracking issue and tag it in the PR.

@aturon
Copy link
Contributor

aturon commented Jan 13, 2017

@clarcharr Let's do it!

@clarfonthey
Copy link
Contributor Author

@aturon It is done!

@aturon
Copy link
Contributor

aturon commented Jan 13, 2017

@bors: r+

Thanks for the PR!

@bors
Copy link
Collaborator

bors commented Jan 13, 2017

📌 Commit e520b77 has been approved by aturon

@bors
Copy link
Collaborator

bors commented Jan 14, 2017

⌛ Testing commit e520b77 with merge 2f9dedb...

bors added a commit that referenced this pull request Jan 14, 2017
expect_err for Result.

This adds an `expect_err` method to `Result`. Considering how `unwrap_err` already exists, this seems to make sense. Inconsistency noted in Manishearth/rust-clippy#1435.
@bors
Copy link
Collaborator

bors commented Jan 14, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 2f9dedb to master...

@bors bors merged commit e520b77 into rust-lang:master Jan 14, 2017
@clarfonthey clarfonthey deleted the expect_err branch January 14, 2017 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants