[PATCH 0/2] Replace unwraps in doctests with graceful handling

[PATCH 0/2] Replace unwraps in doctests with graceful handling

[Daniel Sedlak]

There are multiple occurrences where we use unwraps (or other panickable behavior)
within documentation tests. In the following patches, I try to replace at least some
of the occurrences with graceful handling. I haven't touched the rest (yet),
since I am unsure whether we will all agree on this. If we agree on the handling
I can submit them in v2 or as a follow patches.

Daniel Sedlak (2):
  rust: kernel: init: replace unwraps with the question mark operators
  rust: kernel: rbtree: replace unwraps with functional programming
    paradigms

 rust/kernel/init.rs   |  6 ++--
 rust/kernel/rbtree.rs | 78 ++++++++++++++++++++++---------------------
 2 files changed, 44 insertions(+), 40 deletions(-)

-- 
2.47.0

[PATCH 1/2] rust: kernel: init: replace unwraps with the question mark operators

[Daniel Sedlak]

Link: https://lore.kernel.org/rust-for-linux/CANiq72nsK1D4NuQ1U7NqMWoYjXkqQSj4QuUEL98OmFbq022Z9A@mail.gmail.com/
Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
---
 rust/kernel/init.rs | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 347049df556b..81d69d22090c 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -1076,8 +1076,9 @@ pub fn uninit<T, E>() -> impl Init<MaybeUninit<T>, E> {
 /// ```rust
 /// use kernel::{alloc::KBox, error::Error, init::init_array_from_fn};
 /// let array: KBox<[usize; 1_000]> =
-///     KBox::init::<Error>(init_array_from_fn(|i| i), GFP_KERNEL).unwrap();
+///     KBox::init::<Error>(init_array_from_fn(|i| i), GFP_KERNEL)?;
 /// assert_eq!(array.len(), 1_000);
+/// # Ok::<(), Error>(())
 /// ```
 pub fn init_array_from_fn<I, const N: usize, T, E>(
     mut make_init: impl FnMut(usize) -> I,
@@ -1120,8 +1121,9 @@ pub fn init_array_from_fn<I, const N: usize, T, E>(
 /// ```rust
 /// use kernel::{sync::{Arc, Mutex}, init::pin_init_array_from_fn, new_mutex};
 /// let array: Arc<[Mutex<usize>; 1_000]> =
-///     Arc::pin_init(pin_init_array_from_fn(|i| new_mutex!(i)), GFP_KERNEL).unwrap();
+///     Arc::pin_init(pin_init_array_from_fn(|i| new_mutex!(i)), GFP_KERNEL)?;
 /// assert_eq!(array.len(), 1_000);
+/// # Ok::<(), Error>(())
 /// ```
 pub fn pin_init_array_from_fn<I, const N: usize, T, E>(
     mut make_init: impl FnMut(usize) -> I,
-- 
2.47.0

Re: [PATCH 1/2] rust: kernel: init: replace unwraps with the question mark operators

[Dirk Behme]

Hi Daniel,

On 16.11.24 20:46, Daniel Sedlak wrote:
> Link: https://lore.kernel.org/rust-for-linux/CANiq72nsK1D4NuQ1U7NqMWoYjXkqQSj4QuUEL98OmFbq022Z9A@mail.gmail.com/


While talking about that thread, do you like to cover the dox()
replacement

https://lore.kernel.org/rust-for-linux/459782fe-afca-4fe6-8ffb-ba7c7886de0a@de.bosch.com/

as well?

Thanks for working on this! :)

Dirk


> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
> ---
>  rust/kernel/init.rs | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> index 347049df556b..81d69d22090c 100644
> --- a/rust/kernel/init.rs
> +++ b/rust/kernel/init.rs
> @@ -1076,8 +1076,9 @@ pub fn uninit<T, E>() -> impl Init<MaybeUninit<T>, E> {
>  /// ```rust
>  /// use kernel::{alloc::KBox, error::Error, init::init_array_from_fn};
>  /// let array: KBox<[usize; 1_000]> =
> -///     KBox::init::<Error>(init_array_from_fn(|i| i), GFP_KERNEL).unwrap();
> +///     KBox::init::<Error>(init_array_from_fn(|i| i), GFP_KERNEL)?;
>  /// assert_eq!(array.len(), 1_000);
> +/// # Ok::<(), Error>(())
>  /// ```
>  pub fn init_array_from_fn<I, const N: usize, T, E>(
>      mut make_init: impl FnMut(usize) -> I,
> @@ -1120,8 +1121,9 @@ pub fn init_array_from_fn<I, const N: usize, T, E>(
>  /// ```rust
>  /// use kernel::{sync::{Arc, Mutex}, init::pin_init_array_from_fn, new_mutex};
>  /// let array: Arc<[Mutex<usize>; 1_000]> =
> -///     Arc::pin_init(pin_init_array_from_fn(|i| new_mutex!(i)), GFP_KERNEL).unwrap();
> +///     Arc::pin_init(pin_init_array_from_fn(|i| new_mutex!(i)), GFP_KERNEL)?;
>  /// assert_eq!(array.len(), 1_000);
> +/// # Ok::<(), Error>(())
>  /// ```
>  pub fn pin_init_array_from_fn<I, const N: usize, T, E>(
>      mut make_init: impl FnMut(usize) -> I,

Re: [PATCH 1/2] rust: kernel: init: replace unwraps with the question mark operators

[Daniel Sedlak]

On Sun, Nov 17, 2024 at 8:34 AM Dirk Behme <dirk.behme@gmail.com> wrote:
>
> Hi Daniel,
>
> On 16.11.24 20:46, Daniel Sedlak wrote:
> > Link: https://lore.kernel.org/rust-for-linux/CANiq72nsK1D4NuQ1U7NqMWoYjXkqQSj4QuUEL98OmFbq022Z9A@mail.gmail.com/
>
>
> While talking about that thread, do you like to cover the dox()
> replacement
>
> https://lore.kernel.org/rust-for-linux/459782fe-afca-4fe6-8ffb-ba7c7886de0a@de.bosch.com/
>
> as well?

Thank you for the reminder! Yes, I will incorporate it.
>
> Thanks for working on this! :)
>
> Dirk
>
>
> > Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
> > ---
> >  rust/kernel/init.rs | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> > index 347049df556b..81d69d22090c 100644
> > --- a/rust/kernel/init.rs
> > +++ b/rust/kernel/init.rs
> > @@ -1076,8 +1076,9 @@ pub fn uninit<T, E>() -> impl Init<MaybeUninit<T>, E> {
> >  /// ```rust
> >  /// use kernel::{alloc::KBox, error::Error, init::init_array_from_fn};
> >  /// let array: KBox<[usize; 1_000]> =
> > -///     KBox::init::<Error>(init_array_from_fn(|i| i), GFP_KERNEL).unwrap();
> > +///     KBox::init::<Error>(init_array_from_fn(|i| i), GFP_KERNEL)?;
> >  /// assert_eq!(array.len(), 1_000);
> > +/// # Ok::<(), Error>(())
> >  /// ```
> >  pub fn init_array_from_fn<I, const N: usize, T, E>(
> >      mut make_init: impl FnMut(usize) -> I,
> > @@ -1120,8 +1121,9 @@ pub fn init_array_from_fn<I, const N: usize, T, E>(
> >  /// ```rust
> >  /// use kernel::{sync::{Arc, Mutex}, init::pin_init_array_from_fn, new_mutex};
> >  /// let array: Arc<[Mutex<usize>; 1_000]> =
> > -///     Arc::pin_init(pin_init_array_from_fn(|i| new_mutex!(i)), GFP_KERNEL).unwrap();
> > +///     Arc::pin_init(pin_init_array_from_fn(|i| new_mutex!(i)), GFP_KERNEL)?;
> >  /// assert_eq!(array.len(), 1_000);
> > +/// # Ok::<(), Error>(())
> >  /// ```
> >  pub fn pin_init_array_from_fn<I, const N: usize, T, E>(
> >      mut make_init: impl FnMut(usize) -> I,
>

Daniel

[PATCH 2/2] rust: kernel: rbtree: replace unwraps with functional programming paradigms

[Daniel Sedlak]

Link: https://lore.kernel.org/rust-for-linux/CANiq72nsK1D4NuQ1U7NqMWoYjXkqQSj4QuUEL98OmFbq022Z9A@mail.gmail.com/
Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
---
 rust/kernel/rbtree.rs | 78 ++++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 38 deletions(-)

diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs
index cb4415a12258..3b1436ab0ac3 100644
--- a/rust/kernel/rbtree.rs
+++ b/rust/kernel/rbtree.rs
@@ -36,17 +36,17 @@
 ///
 /// // Check the nodes we just inserted.
 /// {
-///     assert_eq!(tree.get(&10).unwrap(), &100);
-///     assert_eq!(tree.get(&20).unwrap(), &200);
-///     assert_eq!(tree.get(&30).unwrap(), &300);
+///     assert_eq!(tree.get(&10), Some(&100));
+///     assert_eq!(tree.get(&20), Some(&200));
+///     assert_eq!(tree.get(&30), Some(&300));
 /// }
 ///
 /// // Iterate over the nodes we just inserted.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&10, &100));
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
-///     assert_eq!(iter.next().unwrap(), (&30, &300));
+///     assert_eq!(iter.next(), Some((&10, &100)));
+///     assert_eq!(iter.next(), Some((&20, &200)));
+///     assert_eq!(iter.next(), Some((&30, &300)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
@@ -61,21 +61,23 @@
 /// // Check that the tree reflects the replacement.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&10, &1000));
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
-///     assert_eq!(iter.next().unwrap(), (&30, &300));
+///     assert_eq!(iter.next(), Some((&10, &1000)));
+///     assert_eq!(iter.next(), Some((&20, &200)));
+///     assert_eq!(iter.next(), Some((&30, &300)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
 /// // Change the value of one of the elements.
-/// *tree.get_mut(&30).unwrap() = 3000;
+/// if let Some(element) = tree.get_mut(&30) {
+///     *element = 3000;
+/// }
 ///
 /// // Check that the tree reflects the update.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&10, &1000));
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
-///     assert_eq!(iter.next().unwrap(), (&30, &3000));
+///     assert_eq!(iter.next(), Some((&10, &1000)));
+///     assert_eq!(iter.next(), Some((&20, &200)));
+///     assert_eq!(iter.next(), Some((&30, &3000)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
@@ -85,8 +87,8 @@
 /// // Check that the tree reflects the removal.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
-///     assert_eq!(iter.next().unwrap(), (&30, &3000));
+///     assert_eq!(iter.next(), Some((&20, &200)));
+///     assert_eq!(iter.next(), Some((&30, &3000)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
@@ -128,20 +130,20 @@
 /// // Check the nodes we just inserted.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&10, &100));
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
-///     assert_eq!(iter.next().unwrap(), (&30, &300));
+///     assert_eq!(iter.next(), Some((&10, &100)));
+///     assert_eq!(iter.next(), Some((&20, &200)));
+///     assert_eq!(iter.next(), Some((&30, &300)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
 /// // Remove a node, getting back ownership of it.
-/// let existing = tree.remove(&30).unwrap();
+/// let existing = tree.remove(&30);
 ///
 /// // Check that the tree reflects the removal.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&10, &100));
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
+///     assert_eq!(iter.next(), Some((&10, &100)));
+///     assert_eq!(iter.next(), Some((&20, &200)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
@@ -155,9 +157,9 @@
 /// // Check that the tree reflect the new insertion.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&10, &100));
-///     assert_eq!(iter.next().unwrap(), (&15, &150));
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
+///     assert_eq!(iter.next(), Some((&10, &100)));
+///     assert_eq!(iter.next(), Some((&15, &150)));
+///     assert_eq!(iter.next(), Some((&20, &200)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
@@ -677,7 +679,7 @@ fn drop(&mut self) {
 /// Nodes adjacent to the current node can also be removed.
 ///
 /// ```
-/// use kernel::{alloc::flags, rbtree::RBTree};
+/// use kernel::{alloc::flags, rbtree::{RBTree, RBTreeNode, Cursor}};
 ///
 /// // Create a new tree.
 /// let mut tree = RBTree::new();
@@ -688,31 +690,31 @@ fn drop(&mut self) {
 /// tree.try_create_and_insert(30, 300, flags::GFP_KERNEL)?;
 ///
 /// // Get a cursor to the first element.
-/// let mut cursor = tree.cursor_front().unwrap();
-/// let mut current = cursor.current();
-/// assert_eq!(current, (&10, &100));
+/// let mut cursor = tree.cursor_front();
+/// let mut current = cursor.as_ref().map(Cursor::current);
+/// assert_eq!(current, Some((&10, &100)));
 ///
 /// // Calling `remove_prev` from the first element returns [`None`].
-/// assert!(cursor.remove_prev().is_none());
+/// assert!(cursor.as_mut().map(Cursor::remove_prev).flatten().is_none());
 ///
 /// // Get a cursor to the last element.
-/// cursor = tree.cursor_back().unwrap();
-/// current = cursor.current();
-/// assert_eq!(current, (&30, &300));
+/// cursor = tree.cursor_back();
+/// current = cursor.as_ref().map(Cursor::current);
+/// assert_eq!(current, Some((&30, &300)));
 ///
 /// // Calling `remove_prev` removes and returns the middle element.
-/// assert_eq!(cursor.remove_prev().unwrap().to_key_value(), (20, 200));
+/// assert_eq!(cursor.as_mut().map(Cursor::remove_prev).flatten().map(RBTreeNode::to_key_value), Some((20, 200)));
 ///
 /// // Calling `remove_next` from the last element returns [`None`].
-/// assert!(cursor.remove_next().is_none());
+/// assert!(cursor.as_mut().map(Cursor::remove_next).flatten().is_none());
 ///
 /// // Move to the first element
-/// cursor = cursor.move_prev().unwrap();
-/// current = cursor.current();
-/// assert_eq!(current, (&10, &100));
+/// cursor = cursor.map(Cursor::move_prev).flatten();
+/// current = cursor.as_ref().map(Cursor::current);
+/// assert_eq!(current, Some((&10, &100)));
 ///
 /// // Calling `remove_next` removes and returns the last element.
-/// assert_eq!(cursor.remove_next().unwrap().to_key_value(), (30, 300));
+/// assert_eq!(cursor.as_mut().map(Cursor::remove_next).flatten().map(RBTreeNode::to_key_value), Some((30, 300)));
 ///
 /// # Ok::<(), Error>(())
 ///
-- 
2.47.0

Re: [PATCH 2/2] rust: kernel: rbtree: replace unwraps with functional programming paradigms

[Miguel Ojeda]

On Sat, Nov 16, 2024 at 8:52 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> -///     assert_eq!(tree.get(&10).unwrap(), &100);
> -///     assert_eq!(tree.get(&20).unwrap(), &200);
> -///     assert_eq!(tree.get(&30).unwrap(), &300);
> +///     assert_eq!(tree.get(&10), Some(&100));
> +///     assert_eq!(tree.get(&20), Some(&200));
> +///     assert_eq!(tree.get(&30), Some(&300));

This looks fine, in the sense that it is simpler (less characters),
more explicit for the reader (on what `get()` returns) and avoids the
`unwrap()`. Plus some examples of the standard library examples do it
this way too.

However, I am not sure we would necessarily want to do it in all cases
inside `assert!`s. After all, `assert!`s are already panics themselves
(in normal code, and diverging in doctests anyway), i.e. it is
something one would normally not do anyway.

When I suggested this, I was mainly thinking about other places that
we may have `unwrap()`s in example code that could be easily
misunderstood for "normal code", i.e. places that should probably use
`?` or handle the error somehow, like the one in your other patch.

So I am ambivalent about some of the other cases. On one hand, it
makes some tests noisier. On the other hand, it may help to show how
one could do things without `unwrap()`.

Thoughts?

> -/// *tree.get_mut(&30).unwrap() = 3000;
> +/// if let Some(element) = tree.get_mut(&30) {
> +///     *element = 3000;
> +/// }

This change looks wrong to me (not equivalent), because it is changing
the semantics -- this one continues if it couldn't get it, rather than
stopping.

Shouldn't it be `.ok_or(E...)?` or similar (or `EBUG` if we had it,
like Rasmus suggested) if we really wanted to avoid the `.unwrap()`?

By the way, please double check with `CLIPPY=1` (it warns); and if
there is a new version, please Cc also e.g. the author, in this case
Matt (typically one would see what `scripts/get_maintainers.pl`
returns).

Thanks!

Cheers,
Miguel

Re: [PATCH 2/2] rust: kernel: rbtree: replace unwraps with functional programming paradigms

[Daniel Sedlak]

On Sat, Nov 16, 2024 at 10:28 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sat, Nov 16, 2024 at 8:52 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
> >
> > -///     assert_eq!(tree.get(&10).unwrap(), &100);
> > -///     assert_eq!(tree.get(&20).unwrap(), &200);
> > -///     assert_eq!(tree.get(&30).unwrap(), &300);
> > +///     assert_eq!(tree.get(&10), Some(&100));
> > +///     assert_eq!(tree.get(&20), Some(&200));
> > +///     assert_eq!(tree.get(&30), Some(&300));
>
> This looks fine, in the sense that it is simpler (less characters),
> more explicit for the reader (on what `get()` returns) and avoids the
> `unwrap()`. Plus some examples of the standard library examples do it
> this way too.
>
> However, I am not sure we would necessarily want to do it in all cases
> inside `assert!`s. After all, `assert!`s are already panics themselves
> (in normal code, and diverging in doctests anyway), i.e. it is
> something one would normally not do anyway.
>
> When I suggested this, I was mainly thinking about other places that
> we may have `unwrap()`s in example code that could be easily
> misunderstood for "normal code", i.e. places that should probably use
> `?` or handle the error somehow, like the one in your other patch.
>
> So I am ambivalent about some of the other cases. On one hand, it
> makes some tests noisier. On the other hand, it may help to show how
> one could do things without `unwrap()`.
>
> Thoughts?

My reasoning for doing it this way is that people typically tend to copy & paste
code from the examples (citation needed). If we leave the unwraps there, I think
we can convey the wrong impression, that it is OK to use unwraps and this may
be opinionated, but I think™ that examples should also tell that code should not
use unwraps unless the programmer has a really good reason to use them.
Furthermore, unwrap & expect can be used on both Option and Result, and
since there are calls to tree.get(…).unwrap() I wanted to disambiguate
it a little.

But if you think that it is unnecessary, then I will only tackle the
previous scope
of this issue as you mentioned.

>
> > -/// *tree.get_mut(&30).unwrap() = 3000;
> > +/// if let Some(element) = tree.get_mut(&30) {
> > +///     *element = 3000;
> > +/// }
>
> This change looks wrong to me (not equivalent), because it is changing
> the semantics -- this one continues if it couldn't get it, rather than
> stopping.
>
> Shouldn't it be `.ok_or(E...)?` or similar (or `EBUG` if we had it,
> like Rasmus suggested) if we really wanted to avoid the `.unwrap()`?

You are right, it probably should, I will check that and try to incorporate it.
>
> By the way, please double check with `CLIPPY=1` (it warns); and if
> there is a new version, please Cc also e.g. the author, in this case
> Matt (typically one would see what `scripts/get_maintainers.pl`
> returns).

I am not sure, whether I am using the clippy right, I am compiling with
`make LLVM=1 CLIPPY=1`, however, no clippy warnings are triggered.

```text
clippy-driver -V
clippy 0.1.82 (f6e511e 2024-10-15)
```

Also sorry for my question, but which new version of what do you mean? Clippy?

>
> Thanks!
>
> Cheers,
> Miguel

Thank you for your valuable insights!

Daniel

Re: [PATCH 2/2] rust: kernel: rbtree: replace unwraps with functional programming paradigms

[Miguel Ojeda]

On Sun, Nov 17, 2024 at 7:36 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> But if you think that it is unnecessary, then I will only tackle the
> previous scope
> of this issue as you mentioned.

Let's see what others think.

(To clarify, I am happy with the change I quoted for instance)

> I am not sure, whether I am using the clippy right, I am compiling with
> `make LLVM=1 CLIPPY=1`, however, no clippy warnings are triggered.

That should work -- are you building the doctests? (i.e.
`CONFIG_RUST_KERNEL_DOCTESTS`).

The warnings I am getting are `clippy::map_flatten` ones.

> Also sorry for my question, but which new version of what do you mean? Clippy?

I meant the future version of this patch, i.e. v2.

> Thank you for your valuable insights!

You're welcome :)

By the way, for v2, the patches need to have some description in the
commit message (apart from the title and the tags, I mean). Thanks!

Cheers,
Miguel

Re: [PATCH 2/2] rust: kernel: rbtree: replace unwraps with functional programming paradigms

[Daniel Sedlak]

On Sun, Nov 17, 2024 at 9:33 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sun, Nov 17, 2024 at 7:36 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
> >
> > But if you think that it is unnecessary, then I will only tackle the
> > previous scope
> > of this issue as you mentioned.
>
> Let's see what others think.
>
> (To clarify, I am happy with the change I quoted for instance)
>
> > I am not sure, whether I am using the clippy right, I am compiling with
> > `make LLVM=1 CLIPPY=1`, however, no clippy warnings are triggered.
>
> That should work -- are you building the doctests? (i.e.
> `CONFIG_RUST_KERNEL_DOCTESTS`).

Yes, that did the trick. I somewhat assumed that it was turned on by default.
Thank you for helping with that.
>
> The warnings I am getting are `clippy::map_flatten` ones.
>
> > Also sorry for my question, but which new version of what do you mean? Clippy?
>
> I meant the future version of this patch, i.e. v2.
>
> > Thank you for your valuable insights!
>
> You're welcome :)
>
> By the way, for v2, the patches need to have some description in the
> commit message (apart from the title and the tags, I mean). Thanks!
>
> Cheers,
> Miguel

Daniel

Re: [PATCH 0/2] Replace unwraps in doctests with graceful handling

[Miguel Ojeda]

On Sat, Nov 16, 2024 at 8:52 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> There are multiple occurrences where we use unwraps (or other panickable behavior)
> within documentation tests. In the following patches, I try to replace at least some
> of the occurrences with graceful handling. I haven't touched the rest (yet),
> since I am unsure whether we will all agree on this. If we agree on the handling
> I can submit them in v2 or as a follow patches.

Thanks!

Tip for next time: when you are posting patches for comments only,
i.e. when a patch is not meant to be applied as-is because it needs
some discussion first, like in this case, you would typically send it
as an RFC instead -- use `--rfc` in `git format-patch`.

Moreover, ideally you would also add the rationale in the cover letter
(and commit messages), i.e. why you think this should be done (the
Link you had in the other messages is good, but normally you want to
say something rather than just link).

Cheers,
Miguel

Re: [PATCH 0/2] Replace unwraps in doctests with graceful handling

[Daniel Sedlak]

On Sat, Nov 16, 2024 at 9:01 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sat, Nov 16, 2024 at 8:52 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
> >
> > There are multiple occurrences where we use unwraps (or other panickable behavior)
> > within documentation tests. In the following patches, I try to replace at least some
> > of the occurrences with graceful handling. I haven't touched the rest (yet),
> > since I am unsure whether we will all agree on this. If we agree on the handling
> > I can submit them in v2 or as a follow patches.
>
> Thanks!
>
> Tip for next time: when you are posting patches for comments only,
> i.e. when a patch is not meant to be applied as-is because it needs
> some discussion first, like in this case, you would typically send it
> as an RFC instead -- use `--rfc` in `git format-patch`.

Thank you for the tip, I didn't know about this.
>
> Moreover, ideally you would also add the rationale in the cover letter
> (and commit messages), i.e. why you think this should be done (the
> Link you had in the other messages is good, but normally you want to
> say something rather than just link).

Sure, I will add comments to the individual commits. Also, since all
patches try to remove unwraps, I am wondering whether it should
be one large patch or multiple small patches?
>
> Cheers,
> Miguel

Daniel

Re: [PATCH 0/2] Replace unwraps in doctests with graceful handling

[Miguel Ojeda]

On Sun, Nov 17, 2024 at 6:17 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> Sure, I will add comments to the individual commits. Also, since all
> patches try to remove unwraps, I am wondering whether it should
> be one large patch or multiple small patches?

It depends, e.g. sometimes it is split by subsystem, so that you can
address different sets of people and so that they can take the patches
independently within their own schedules and through their trees.

You can search for some big/tree-wide series to see examples on how it
is usually done, or read e.g. https://lwn.net/Articles/585782/

I hope that helps!

Cheers,
Miguel