-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Add section on common message styles for Result::expect #96033
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
Changes from 7 commits
dfb37cb
3d951b3
80c362b
5d98acb
eca7dd2
00a315c
72898ac
7b5dce9
9a9dafc
b6b621e
720e987
ef879c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1023,6 +1023,80 @@ impl<T, E> Result<T, E> { | |
/// let x: Result<u32, &str> = Err("emergency failure"); | ||
/// x.expect("Testing expect"); // panics with `Testing expect: emergency failure` | ||
/// ``` | ||
/// | ||
/// # Common Message Styles | ||
/// | ||
/// There are two common styles for how people word `expect` messages. Using | ||
/// the message to present information to users encountering a panic | ||
/// ("expect as error message") or using the message to present information | ||
/// to developers debugging the panic ("expect as precondition"). | ||
/// | ||
/// In the former case the expect message is used to describe the error that | ||
/// has occurred which is considered a bug. Consider the following example: | ||
/// | ||
/// ```should_panic | ||
/// // Read environment variable, panic if it is not present | ||
/// let path = std::env::var("IMPORTANT_PATH").unwrap(); | ||
/// ``` | ||
/// | ||
/// In the "expect as error message" style we would use expect to describe | ||
/// that the environment variable was not set when it should have been: | ||
/// | ||
/// ```should_panic | ||
/// let path = std::env::var("IMPORTANT_PATH") | ||
/// .expect("env variable `IMPORTANT_PATH` is not set"); | ||
/// ``` | ||
/// | ||
/// In the "expect as precondition" style, we would instead describe the | ||
/// reason we _expect_ the `Result` should be `Ok`. With this style we would | ||
/// prefer to write: | ||
/// | ||
/// ```should_panic | ||
/// let path = std::env::var("IMPORTANT_PATH") | ||
/// .expect("env variable `IMPORTANT_PATH` should be set by `wrapper_script.sh`"); | ||
/// ``` | ||
/// | ||
/// The "expect as error message" style does not work as well with the | ||
/// default output of the std panic hooks, and often ends up repeating | ||
/// information that is already communicated by the source error being | ||
/// unwrapped: | ||
/// | ||
/// ```text | ||
/// thread 'main' panicked at 'env variable `IMPORTANT_PATH` is not set: NotPresent', src/main.rs:4:6 | ||
/// ``` | ||
/// | ||
/// In this example we end up mentioning that an env variable is not set, | ||
/// followed by our source message that says the env is not present, the | ||
/// only additional information we're communicating is the name of the | ||
/// environment variable being checked. | ||
/// | ||
/// The "expect as precondition" style instead focuses on source code | ||
/// readability, making it easier to understand what must have gone wrong in | ||
/// situations where panics are being used to represent bugs exclusively. | ||
/// Also, by framing our expect in terms of what "SHOULD" have happened to | ||
/// prevent the source error, we end up introducing new information that is | ||
/// independent from our source error. | ||
/// | ||
/// ```text | ||
/// thread 'main' panicked at 'env variable `IMPORTANT_PATH` should be set by `wrapper_script.sh`: NotPresent', src/main.rs:4:6 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In an ideal world this example would either use For now I'm just letting it print poorly rather than complicating the example with an adapter type to convert |
||
/// ``` | ||
/// | ||
/// In this example we are communicating not only the name of the | ||
/// environment variable that should have been set, but also an explanation | ||
/// for why it should have been set, and we let the source error display as | ||
/// a clear contradiction to our expectation. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the strong language here, because it also fits well with a more general thing about saying that |
||
/// | ||
/// For programs where panics may be user facing, either style works best | ||
/// when paired with a custom [panic hook] like the one provided by the CLI | ||
/// working group library, [`human-panic`]. This panic hook dumps the panic | ||
/// messages to a crash report file while showing users a more friendly | ||
/// "Oops, something went wrong!" message with a suggestion to send the | ||
/// crash report file back to the developers. Panic messages should be used | ||
/// to represent bugs, and the information provided back is context intended | ||
/// for the developer, not the user. | ||
/// | ||
/// [panic hook]: https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html | ||
/// [`human-panic`]: https://docs.rs/human-panic | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#[inline] | ||
#[track_caller] | ||
#[stable(feature = "result_expect", since = "1.4.0")] | ||
|
Uh oh!
There was an error while loading. Please reload this page.