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

Extra methods for Option #1597

Open
ghost opened this issue Apr 28, 2016 · 14 comments
Open

Extra methods for Option #1597

ghost opened this issue Apr 28, 2016 · 14 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@ghost
Copy link

ghost commented Apr 28, 2016

I noticed that a few potentially useful methods convertingOption to Result aren't currently implemented: ok_or_default, err_or, err_or_else, and err_or_default.

ok_or_default could be useful as a complement to unwrap_or_default when converting Options to Results with a certain error type.

All of the err_or functions are complements to ok_or and aren't implemented at all.

@ticki
Copy link
Contributor

ticki commented Apr 29, 2016

Also, unchecked_unwrap() and give (opposite of take).

@ticki
Copy link
Contributor

ticki commented Apr 29, 2016

@oli-obk
Copy link
Contributor

oli-obk commented Apr 30, 2016

  • the opposite of take is *opt = value;, there's no reason for a method.
  • unchecked_unwrap has been discussed and the comments suggest that you should rather stabilize std::intrinsics::unreachable
  • ok_or_default can be expressed as ok_or_else(Default::default)
  • what's the meaning of err_or_*? turn Some(x) into Err(x)? That's very confusing, please us a match
  • do you have statistical evidence that those functions will carry their weight? I would have wanted some of them sometimes, but in hindsight those situations are so rare, an explicit match is much better, because the future reader (myself included) will instantly recognize this situation as "something hokey pokey is going on, pay attention while reading"

@ticki
Copy link
Contributor

ticki commented Apr 30, 2016

the opposite of take is *opt = value;, there's no reason for a method.

No, you might misunderstand what I want.

pub fn give(&mut self, val: T) -> Option<T> {
    ::std::mem::replace(self, Some(val))
}

do you have statistical evidence that those functions will carry their weight? I would have wanted some of them sometimes, but in hindsight those situations are so rare, an explicit match is much better, because the future reader (myself included) will instantly recognize this situation as "something hokey pokey is going on, pay attention while reading"

(assuming you're referencing libextra) Yes, we do use those a lot. They are a better and faster alternative to panicking, especially when making CLI tools.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 30, 2016

Wouldn't you get the same behaviour with abort on panic? Or is this a choice that varies inside a single project?

The other cases are what i meant actually, are there lots of awkward matches when converting options to results?

@ticki
Copy link
Contributor

ticki commented Apr 30, 2016

Wouldn't you get the same behaviour with abort on panic? Or is this a choice that varies inside a single project?

Nope, because of the way errors are handled and written to stderr. Abort on panic doesn't change that.

@ticki
Copy link
Contributor

ticki commented Apr 30, 2016

For example, we don't want coreutils to do thread '<main>' paniced at: "blah", we want to have simply blah.

@leoyvens
Copy link

#1416 for previous discussion on give. I'm against it because after being called it would guarantee that the Option is Some which encourages use of unwrap.

@nrc nrc added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Aug 18, 2016
@Kerollmops
Copy link
Contributor

Why not something like the new RefCell::replace method ?

@F001
Copy link
Contributor

F001 commented Jan 15, 2018

@Kerollmops RefCell::replace and Cell::replace are special because of interior mutability , so they only require &self receiver.

Whereas, for Option type, the replace method must take &mut self receiver, which makes it not necessary, in which case we can use assignment directly or std::mem::replace.

@F001
Copy link
Contributor

F001 commented Jan 15, 2018

But I do wonder Option can support placement-in protocol rust-lang/rust#30172. opt <- item seems reasonable.

@Kerollmops
Copy link
Contributor

Kerollmops commented Jan 15, 2018

I think the same kind of reflection can be applied to the Option::map, it is useless, Option implement IntoIterator so instead of my_option.map(|v| func) we can do my_option.into_iter().map(|v| func) but it's more convenient to use a map method directly.

So yes we can use mem::replace but it can be more convenient to use a Option::replace method like the Option::take one.

What do you think ?

@F001
Copy link
Contributor

F001 commented Jan 15, 2018

Yes. It makes sense. Option::take is already there. No reason to not have Option::replace.

@Kerollmops
Copy link
Contributor

Kerollmops commented Jan 15, 2018

These two methods have not the same behaviors:

  • Option::take will pop the value and return it if present, let a None in place, it takes only &mut self and no other argument.
  • Option::replace should interchange values and return the previous value if present, let a Some in place, it takes &mut self and one argument.

EDIT: doesn't correctly read your answer, you are on my side, sorry.

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 RFC.
Projects
None yet
Development

No branches or pull requests

6 participants