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

Add unwrap_or_default method to Result #37299

Merged
merged 4 commits into from
Nov 1, 2016
Merged

Add unwrap_or_default method to Result #37299

merged 4 commits into from
Nov 1, 2016

Conversation

devonhollowood
Copy link
Contributor

Fixes #37025

@rust-highfive
Copy link
Collaborator

r? @sfackler

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

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 20, 2016
killercup
killercup reviewed Oct 20, 2016
View reviewed changes
Copy link
Member

@killercup killercup left a comment

Choose a reason for hiding this comment

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

Nice! I've wanted to use this (but needed to write .unwrap_or_else(Default::default) instead) :)

src/libcore/result.rs
///
/// Convert a string to an integer, turning poorly-formed strings
/// into 0 (the default value for integers). `parse` converts
/// a string to any other type that implements `FromStr`, returning an
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to link FromStr to the FromStr docs page

src/libcore/result.rs
/// assert_eq!(0, bad_year);
/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
Copy link
Member

Choose a reason for hiding this comment

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

Is this stable attribute correct? I'd expect it to have since = "1.14.0" (I don't know where feature names are defined).

Copy link
Member

Choose a reason for hiding this comment

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

This should probably start as unstable.

Copy link
Member

Choose a reason for hiding this comment

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

instruction: Should be unstable, no version number. Pick a new feature name, for example "result_unwrap_or_default". Use issue = "0" in the unstable tag; an issue can be opened and filled in when this PR has been cleared for inclusion.

@leoyvens
Copy link
Contributor

Pros:

  1. It's symmetrical with Option.
  2. It's very ergonomic.

Cons:

  1. It can already be achieved in 2 ergonomic ways, unwrap_or_else(Default::default) and unwrap_or(Default::default()).
  2. Ergonomic way to throw away the error.
  3. It' more API surface for Result that already has many convenience methods, do we really want all the Option stuff in Result as well?

@alexcrichton
Copy link
Member

Thanks for the PR @devonhollowood! I agree that we'll want to start out here with an #[unstable] method instead of #[stable] and then this can go through the normal stabilization process to enter stable Rust.

Also thanks for the pros/cons list @leodasvacas! I personally feel that this is filling a gap where Option and Result are expected to have very similar APIs, so I'd be fine adding this.

@devonhollowood
Copy link
Contributor Author

Apologies about the stability thing. Somewhat embarrassingly, that line of code got caught up when I was copying the documentation example from Option::unwrap_or_default(), since I thought that it would be nice to have some symmetry in the docs between the two methods. Then I think I just didn't notice it afterwards. Anyways, I changed it to unstable (feature = "result_unwrap_or_default"), and I'm rebuilding the docs now to check that the links to parse and FromStr work (as @killercup suggested). Once that's done, I'll push the updated commit.

Incidentally, is there a faster way to build the docs than make doc? It seems to be taking longer than I would have expected.

@alexcrichton
Copy link
Member

Looks great to me, thanks! We'll want to change the issue number of 0 to something real eventually, but for now that's fine. In the meantime...

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Oct 22, 2016 •
edited
Loading

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@arthurprs
Copy link
Contributor

Good addition, thanks.

@rfcbot
Copy link

rfcbot commented Oct 31, 2016

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 31, 2016

📌 Commit 5d31a81 has been approved by alexcrichton

@jseyfried jseyfried added relnotes Marks issues that should be documented in the release notes of the next release. and removed relnotes Marks issues that should be documented in the release notes of the next release. labels Nov 1, 2016
@bors
Copy link
Contributor

bors commented Nov 1, 2016

⌛ Testing commit 5d31a81 with merge ac968c4...

bors added a commit that referenced this pull request Nov 1, 2016
@bors
Auto merge of #37299 - devonhollowood:result-unwrap-or-default, r=ale…
ac968c4 
…xcrichton

Add `unwrap_or_default` method to `Result`

Fixes #37025
@bors
Copy link
Contributor

bors commented Nov 1, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: alexcrichton
Pushing ac968c4 to master...

@bors bors merged commit 5d31a81 into rust-lang:master Nov 1, 2016
@bors bors mentioned this pull request Nov 1, 2016
Merged
This was referenced Nov 1, 2016
Closed
Merged
sophiajt pushed a commit to sophiajt/rust that referenced this pull request Nov 2, 2016
Rollup merge of rust-lang#37517 - Mark-Simulacrum:add-unwrap-default-…
805aecc 
…tracking-issue, r=alexcrichton

Add tracking issue number to Result::unwrap_or_default unstable annotation.

Implemented in rust-lang#37299.

Tracking issue: rust-lang#37516.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@killercup killercup killercup left review comments

@retep998 retep998 retep998 left review comments

@bluss bluss bluss left review comments

Assignees

@sfackler sfackler

Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.