Radish alpha
h
Radicle Heartwood Protocol & Stack
Radicle
Git (anonymous pull)
Log in to clone via SSH
CONTRIBUTING: reword section on unwrap and expect
Fintan Halpenny committed 3 months ago
commit c1fddf078d1e0de1e271f35d784ed1ce81790e75
parent 7ba6b5a57b38e968c0d8aa5d6eda84ea58d86cbf
1 file changed +15 -25
modified CONTRIBUTING.md
@@ -84,31 +84,21 @@ The following code guidelines will help make code review smoother.

#### Use of `unwrap` and `expect`

-
Use `unwrap` only in either of three circumstances:
-

-
1. Based on manual static analysis, you've concluded that it's impossible for
-
the code to panic; so unwrapping is *safe*. An example would be:
-

-
        let list = vec![a, b, c];
-
        let first = list.first().unwrap();
-

-
2. The panic caused by `unwrap` would indicate a bug in the software, and it
-
would be impossible to continue in that case.
-

-
3. The `unwrap` is part of test code, ie. `cfg!(test)` is `true`.
-

-
In the first and second case, document `unwrap` call sites with a comment prefixed
-
with `SAFETY:` that explains why it's safe to unwrap, eg.
-

-
    // SAFETY: Node IDs are valid ref strings.
-
    let r = RefString::try_from(node.to_string()).unwrap();
-

-
Use `expect` only if the function expects certain invariants that were not met,
-
either due to bad inputs, or a problem with the environment; and include the
-
expectation in the message. For example:
-

-
    logger::init(log::Level::Debug)
-
        .expect("logger must only be initialized once");
+
In general, `unwrap` or `expect` should be seldom used.
+
Instead, proper handling of these cases should made, whether through better expressing
+
the logic using types, through error handling, or a combination of these.
+

+
However, there are some instances where they are acceptable:
+

+
1. In tests, where `#[allow(clippy::unwrap_used)]` is commonly used.
+
2. If, after analysing the code for yourself, you have concluded that
+
   a call to `unwrap` does not panic (not under any circumstance, for any given
+
   input). In this case, it is still preferred to use `expect` and provide
+
   a message which gives your argument.
+
   Note that if your analysis does not appear very stable under possible future
+
   refactorings and changes, it might still not be accepted.
+
3. The presence of `Option::None` or `Result::Err` is truly an unexpected scenario
+
   and you intend the program to panic.

#### Module imports