-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Put glaring warnings and many capital letters in the docs of option::get() and option::unwrap(). #3606
Comments
This seems like a good use for typestate. |
I bet the system we had supported this pretty well :) |
I remember myself using I dunno. I'm not sure it's worth more language features to track in the cases where you actually do need to be imperative about it. |
I admit to using unwrap a lot - Rust's the first language that I've seen Option types in (the only other functional language I really know is Erlang). I've just done a grep through the code and seen how it's supposed to be, and now feel embarrassed. Those who aren't exposed to the correct way of doing it are likely to do it wrong - I support the idea of making the documentation a bit more explicit about the correct way of getting things out of Options |
Is there ever a legitimate reason to use |
I think Still, using |
While I think Hence the suggestion to add docs rather than a small extra language feature (also perhaps rather than removing get() from the library altogether, which would be terrible). |
@catamorphism I can't think of any. I used to use I do think |
FWIW the if-based code is smaller and introduces only one new indentation level instead of two:
vs
|
In my experience, I would write The pattern `if is.is_none() { /* do nothing; return */ } mimics the early returns that are common in C++ code like SpiderMonkey. In both cases, the intention is reduce indentation and the associated cognitive load. |
@Dretch:
|
@erickt I think maybe what he meant was something like
which is super "imperative". |
I beefed up the docs a little in c83218d by adding
|
That looks super. |
Rustup, aligned heap-allocations on wasm Pulls in rust-lang#125003 so allocation tests should now work on wasm.
I realised this when replying to @eholk on my blog: http://winningraceconditions.blogspot.com/2012/09/rust-0-index-and-conclusion.html?showComment=1348721405975#c9047445512681989061
Basically, I worry that novices who are familiar with null pointer checks but not familiar with the functional "option" idiom will start learning rust, then pop into the channel or go to their friends asking "How do I get the
T
out of anOption<T>
??", and start usingget()
orunwrap()
indiscriminately, when really they should be learning aboutmatch
and how to properly handle theNone
case.I think a big step here would be to write something like "WARNING: If you are not SURE that the optional value is not None, use a match statement instead and handle the None case explicitly!" in the documentation for these functions.
The text was updated successfully, but these errors were encountered: