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

option: rename unwrap to get or remove get_ref in favour of as_ref().unwrap() #9784

Closed
thestinger opened this issue Oct 9, 2013 · 14 comments
Assignees
Milestone

Comments

@thestinger
Copy link
Contributor

There are already nearly identical methods named get_ref() and get_mut_ref() and std::hashmap names the equivalent method get. This method doesn't always move, it's just a normal by-value method, so it should follow the same convention as other get-or-fail methods. If it used the shorter naming, replacing get_mut_ref() with as_mut().get() would only add a character.

@alexcrichton
Copy link
Member

I was under the impression that we were going to rename get_mut_ref to get_mut to align with as_{ref,mut}?

I also believe that the reason for leaving these two methods instead of making them composable was because they're likely very common patterns.

@thestinger
Copy link
Contributor Author

Nominated for library backwards compatibility tag.

@nikomatsakis
Copy link
Contributor

+1 for unwrap() => get()

@catamorphism
Copy link
Contributor

If we rename, other types (like arcs) should be consistent.

1.0, backwards-compat

@pcwalton
Copy link
Contributor

This is fine, but please let's stop changing this. This needs to be the last change and then this method is frozen and changing it is off the table.

@mletterle
Copy link
Contributor

I vote for get(), I think it's more intuitive, especially with get_or() and any other variations.

@Kimundi
Copy link
Member

Kimundi commented Oct 26, 2013

1+ for get(), ever since @thestinger added the Option by-ref adapters this became the most fitting name.

@glaebhoerl
Copy link
Contributor

One thing that's not clear to me: among Haskellers, using fromJust is considered nearly evil, and is avoided whenever possible, while among the Rustic it's so common that making it convenient is a significant consideration. Why is that?

@thestinger
Copy link
Contributor Author

It's better to have a reusable evil method with a concise name than the current situation of many functions preferring to fail than return an Option because people find them too inconvenient. In most application code, not every error case is going to be handled. In library code, you definitely want to expose the ability to handle any error cases, without making common application code painful.

@huonw
Copy link
Member

huonw commented Nov 9, 2013

It sounds like it might be worth going through the stdlib and converting fail!()-ing constructors (and other functions) to ones returning Options or Results where reasonable, since the stdlib will set the tone for other libraries.

I agree with @glehel that Rust seems to be far too free with using .get()/.unwrap()... even .expect("<some useful message>") is better since it makes it (1) obvious that the author has thought a little about the failing case, and (2) gives a slightly useful error message, rather than just called Option::unwrap()on aNone value, which is pathetically useless, and requires diving into GDB to work out exactly where it happened.

As a point against this in Rust, we don't have the functors/applicatives/do-sugar that Haskell has that makes working with these nicer.

@Kimundi
Copy link
Member

Kimundi commented Nov 9, 2013

Also, we very often use use Option in situations where you know it's not going to be a None for the value you're calling the function with.

For example, something like let v: uint = from_str("123").unwrap() will never fail, as will iterative constructs like this:

if opt.is_some() {
    // ...
    let x = opt.unwrap();
    // ...
}

Mutable options also necessitate a lot of wrapping and unwrapping of values, as those cases are basically one-element containers.

@nikomatsakis
Copy link
Contributor

cc #13159

@thestinger thestinger reopened this Jun 17, 2014
@thestinger thestinger changed the title (RFC) option: rename unwrap to get (RFC) option: rename unwrap to get or remove get_ref in favour of as_ref().unwrap() Jun 17, 2014
@thestinger thestinger changed the title (RFC) option: rename unwrap to get or remove get_ref in favour of as_ref().unwrap() option: rename unwrap to get or remove get_ref in favour of as_ref().unwrap() Jun 17, 2014
@thestinger thestinger removed the B-RFC label Jun 17, 2014
@aturon
Copy link
Member

aturon commented Aug 4, 2014

The guidelines now have a convention for these names. I will be rolling them out in a patch soon.

@aturon
Copy link
Member

aturon commented Sep 11, 2014

The PR for Option/Result stabilization resolved this issue by deprecating get_ref, as suggested. Closing.

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 a pull request may close this issue.

10 participants