From 0711591ad3995d0201f830acb06127952c8d4391 Mon Sep 17 00:00:00 2001
From: har7an <99636919+har7an@users.noreply.github.com>
Date: Tue, 18 Oct 2022 14:14:38 +0000
Subject: [PATCH] docs: Describe how to handle Options as errors (#1805)

* docs: Describe how to handle Options as errors

* CONTRIBUTING: Add tips for code contributions

which will be home to condensed tips and best-practices around the
zellij code. Currently explains to prefer returning `Result` types
instead of `unwrap`ing on them.

The tips in here are meant to be short, concise guides that allow the
user to get started without a lot of reading. The individual tips can
(and should) be supplemented with links to "further reading" where the
topic at hand is explained in greater detail.
---
 CONTRIBUTING.md        | 15 ++++++++++++++
 docs/ERROR_HANDLING.md | 44 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 0f62b09e..59159a98 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -113,6 +113,21 @@ something interesting to work on and guide through.
 [discord-invite-link]: https://discord.gg/feHDHahHCz 
 [good-first-issue]: https://github.com/zellij-org/zellij/labels/good%20first%20issue
 
+
+## Tips for Code Contributions
+
+### Prefer returning `Result`s instead of `unwrap`ing
+
+- Add `use zellij_utils::errors::prelude::*;` to the file
+- Make the function return `Result<T>`, with an appropriate `T` (Use `()` if there's nothing to return)
+- Append `.context()` to any `Result` you get with a sensible error description (see [the docs][error-docs-context])
+- Generate ad-hoc errors with `anyhow!(<SOME MESSAGE>)`
+- *Further reading*: [See here][error-docs-result]
+
+[error-docs-context]: https://github.com/zellij-org/zellij/blob/main/docs/ERROR_HANDLING.md#attaching-context
+[error-docs-result]: https://github.com/zellij-org/zellij/blob/main/docs/ERROR_HANDLING.md#converting-a-function-to-return-a-result-type
+
+
 ## Filing Issues
 
 Bugs and enhancement suggestions are tracked as GitHub issues.
diff --git a/docs/ERROR_HANDLING.md b/docs/ERROR_HANDLING.md
index 02ba2983..6304e966 100644
--- a/docs/ERROR_HANDLING.md
+++ b/docs/ERROR_HANDLING.md
@@ -101,6 +101,12 @@ the following text.
 
 ## Converting a function to return a `Result` type
 
+> **TL;DR**  
+> - Add `use zellij_utils::errors::prelude::*;` to the file
+> - Make the function return `Result<T>`, with an appropriate `T` (Often `()`)
+> - Append `.context()` to any `Result` you get with a sensible error description (see below)
+> - Generate ad-hoc errors with `anyhow!(<SOME MESSAGE>)`
+
 Here's an example of the `Screen::render` function as it looked before:
 
 ```rust
@@ -321,6 +327,44 @@ the application (i.e. crash zellij). Since we made sure to attach context
 messages to the errors on their way up, we will see these messages in the
 resulting output!
 
+
+## Error handling for `Option` types
+
+Beyond what's described in "Choosing helpful context messages" above, `Option`
+types benefit from extra handling. That is because a `Option` containing a
+`None` where a value is expected doesn't carry an error message: It doesn't
+tell us why the `None` is bad (i.e. equivalent to an Error) in this case.
+
+In situations where a call to `unwrap()` or similar on a `Option` type is to be
+converted for error handling, it is a good idea to attach an additional short
+context. An example from the zellij codebase is shown below:
+
+```rust
+    let destination_tab = self.get_indexed_tab_mut(destination_tab_index)
+        .context("failed to get destination tab by index")
+        .with_context(|| format!("failed to move clients from tab {source_tab_index} to tab {destination_tab_index}"))?;
+```
+
+Here the call to `self.get_indexed_tab_mut(destination_tab_index)` will return
+a `Option`. The surrounding code, however, doesn't know what to do with a
+`None` value, so it is considered an error.
+
+Here you see that we attach two contexts:
+
+```rust
+        .context("failed to get destination tab by index")
+```
+
+Because the `None` type itself doesn't tell us what the "error" means, we
+attach a description manually. The second context:
+
+```rust
+        .with_context(|| format!("failed to move clients from tab {source_tab_index} to tab {destination_tab_index}"))?;
+```
+
+then describes what the surrounding function was trying to achieve (See
+descriptions above).
+
 [tracking_issue]: https://github.com/zellij-org/zellij/issues/1753
 [handle_panic]: https://docs.rs/zellij-utils/latest/zellij_utils/errors/fn.handle_panic.html
 [miette]: https://crates.io/crates/miette