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.

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

Add Result::expect() for consistency with Option::expect(). #11140

Closed
wants to merge 1 commit into from
Closed

Add Result::expect() for consistency with Option::expect(). #11140

wants to merge 1 commit into from

Conversation

Skrylar
Copy link

@Skrylar Skrylar commented Dec 25, 2013

Adds an "expect(M)" call to Result, which makes it consistent with Option.

@alexcrichton
Copy link
Member

The expect method has previously not existed on the Result type because you have an obvious error message (the Err value). In the past the error value had the ToStr bound so a good message could be presented as part of unwrap, but that seems to have gone away.

@Skrylar
Copy link
Author

Skrylar commented Dec 25, 2013

@alexcrichton I mostly put this up because unwrap() doesn't actually use the error message; we were talking about the need for a method which did exactly that: unwraps and throws the error itself on failure. But there isn't one, and I'm not sure what to call a method that does exactly that. I made a local version with traits named .assert(), but that doesn't feel correct either.

@alexcrichton
Copy link
Member

What I was getting at is that this may not be the right solution for the problem at hand. You are correct in that unwrap() doesn't have an error message, but that may be the bug here rather than adding yet another method to result/option.

Regardless, this needs discussion for the direction to go in. I'm personally a fan of keeping the apis small and putting an extra bound on the error type.

@Kimundi
Copy link
Member

Kimundi commented Dec 26, 2013

This method is unnecessary with the option adapters - res.ok().expect() does exactly what this method would do.

There has been a huge discussion in the past (mainly initiated by me) of whether the Result API should mirror the Option API, or whether it should instead offer option adapter methods (ok() -> Option<T>, err() -> Option<E>).

Both have pros and cons: Duplicating the API means redefining all Option methods for Result twice, while using option adapters means that the error type gets dropped on the floor in unwrap-like methods.

The option adapter way is was has been decided on, so the Result API now is basically done, plus minus unwrap -> get rename and outside forces.

For more details, see #10364.

@alexcrichton
Copy link
Member

Ah, I had forgotten that! @Kimundi is right, the ok() and err() methods are meant to allow composition with the Option api to avoid blowing up the Result api. Since we have previously made that decision, I'm going to close this.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 31, 2023
New lint [`four_forward_slashes`]

Closes rust-lang#9212

changelog: New lint [`four_forward_slashes`]
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.

3 participants