Skip to content

Made Results API more composable #10364

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.

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

Closed
wants to merge 1 commit into from

Conversation

Kimundi
Copy link
Member

@Kimundi Kimundi commented Nov 8, 2013

This implements parts of the changes to Result and Option I proposed and discussed in this thread: https://mail.mozilla.org/pipermail/rust-dev/2013-November/006254.html

This PR includes:

  • Adding ok() and err() option adapters for both Result variants.
  • Removing get_ref, expect and iterator constructors for Result, as they are reachable with the variant adapters.
  • Removing Results ToStr bound on the error type because of composability issues. (See https://mail.mozilla.org/pipermail/rust-dev/2013-November/006283.html)
  • Some warning cleanups

@alexcrichton
Copy link
Member

Hm, I'd like to discuss this in a meeting because I think we want to settle on the result API now instead of later. Next week we're not having a meeting, so this would have to sit for awhile before we get to officially discuss it.

I'm personally in favor of this change, though, I like the direction it went in!

@alexcrichton
Copy link
Member

I've added this to the minutes of the next meeting.

@alexcrichton
Copy link
Member

erm, agenda

/// Unwraps a result, yielding the content of an `Err`.
/// Fails if the value is an `Ok`.
#[inline]
pub fn err_get(self) -> E {
Copy link
Member

Choose a reason for hiding this comment

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

get_err seems like a better name to me, and more consistent with every other function (get_ptr, get_mut, etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea behind that naming is that err_get is shorthand for err().get(), just like for Option take_unwrap is shorthand for take().unwrap().

Copy link
Contributor

Choose a reason for hiding this comment

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

get_err() is a better name, from a user view

@Kimundi
Copy link
Member Author

Kimundi commented Nov 23, 2013

Did this get discussed at the last meeting?

@alexcrichton
Copy link
Member

This did not, but it'll remain on the agenda until it does. I'm still very much in favor of this, we just need some more opinions to weigh in on this.

@gsemet
Copy link

gsemet commented Nov 27, 2013

I +1 this PR, however, is it possible to write a document about the naming convention to apply in all module? It's more than a Coding Style guide, it's more a general reference page where one would find the convention to apply. Because I cannot look at the different "convention" used in different rust modules to write my own. I've setup this kind of naming / API convention document in my work, and this was quite successful. One would follow this doc for new development, and if a change in the convention occurs (changes will occur), it's more easier to find out "what is good/what is bad" at first sight.

@alexcrichton
Copy link
Member

It was decided in the meeting this week that we'd like to accept this, so feel free to ping me when rebased!

bors added a commit that referenced this pull request Dec 7, 2013
This implements parts of the changes to `Result` and `Option` I proposed and discussed in this thread: https://mail.mozilla.org/pipermail/rust-dev/2013-November/006254.html

This PR includes:
- Adding `ok()` and `err()` option adapters for both `Result` variants.
- Removing `get_ref`, `expect` and iterator constructors for `Result`, as they are reachable with the variant adapters.
- Removing `Result`s `ToStr` bound on the error type because of composability issues. (See https://mail.mozilla.org/pipermail/rust-dev/2013-November/006283.html)
- Some warning cleanups
@bors bors closed this Dec 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants