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 Option::into_result #17469

Merged
merged 1 commit into from
Sep 27, 2014
Merged

Add Option::into_result #17469

merged 1 commit into from
Sep 27, 2014

Conversation

sfackler
Copy link
Member

This is the inverse of Result::ok and helps to bridge Option and
Result based APIs.

@sfackler
Copy link
Member Author

cc @aturon - not sure what stability attribute to put on this.

@aturon
Copy link
Member

aturon commented Sep 23, 2014

Thanks, I'd been wanting to add this API. Please mark as experimental for now.

@aturon
Copy link
Member

aturon commented Sep 23, 2014

I'm wondering about the rationale for returning a Result<T, ()>, rather than taking an explicit error value (or closure) and mapping into a Result<T, E>?

@sfackler
Copy link
Member Author

You can always swap out the error type by composing with Result methods: opt.into_result().map_err(|_| MyError). I could add the error type, but then we'd probably want two variants, one that takes a closure and one that takes a value and I'm not sure the complexity's worth it.

Pushed a version with the stability attribute.

@aturon
Copy link
Member

aturon commented Sep 23, 2014

Right. It's a question of what we expect to be the common usage. The Result<T, ()> type is fairly rare (and uninformative); I'd like to make it ergonomic to get a meaningful error.

I agree that we may want a variant with a closure, but for now adding a parameter for the error component seems like an improvement:

// compare:
opt.into_result()
opt.into_result(())

// compare:
opt.into_result().map_err(|_| MyError)
opt.into_result(MyError)

Also, I'd like to think a bit about the name of this method. The corresponding Result methods are ok and err. Should we try to do something similar here? For example, we could use some_or and some_or_else (if we wanted to have the closure variant).

Finally, we may be able to use a trick from the collections reform RFC to drop all of the closure variants.

@alexcrichton
Copy link
Member

Thinking about it it's interesting that I don't feel the need to prefix ok or err with "into" even though it's consuming the result. This may be because almost all methods on Result/Option consume the receiver, but it's also not actually saying what it's transforming into -- ok does not give you Ok, it gives you Option<T>.

We could possibly drop the into_ prefix for just result because most other methods of Option don't have the prefix, but foo.result(bar) isn't quite so informative... I do agree with @aturon that taking the E of the Result as an argument is probably a good idea.

@sfackler
Copy link
Member Author

foo.into_result(MyError) seems weird. foo.ok_or(MyError) and foo.ok_or_else(|| MyError) seem decently readable and match the Option API. Those sound good?

@aturon
Copy link
Member

aturon commented Sep 26, 2014

@sfackler So, Result gives you ok as a way to "project" out the Ok variant, whereas you're using ok here as "injecting" into the Result type. It seems like some_or and some_or_else would be the analogous thing here, if we want to be consistent.

That said, I think ok_or(MyError) reads really nicely, so maybe we should just go with it and not worry about the minor inconsistency.

In any case, I'm glad to land this with either name as #[experimental] and we can finalize the name during a later stabilization pass.

@sfackler
Copy link
Member Author

Updated

These are the inverses of `Result::ok` and help to bridge `Option` and
`Result` based APIs.
bors added a commit that referenced this pull request Sep 27, 2014
This is the inverse of `Result::ok` and helps to bridge `Option` and
`Result` based APIs.
@bors bors closed this Sep 27, 2014
@bors bors merged commit 0c8878d into rust-lang:master Sep 27, 2014
@sfackler sfackler deleted the into-result branch November 26, 2016 05:52
lnicola pushed a commit to lnicola/rust that referenced this pull request Jun 23, 2024
fix: use ItemInNs::Macros to convert ModuleItem to ItemInNs

fix rust-lang#17425.

When converting `PathResolution` to `ItemInNs`, we should convert `ModuleDef::Macro` to `ItemInNs::Macros` to ensure that it can be found in `DefMap`.
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.

4 participants