Revise exit status section

[codesections]

This PR revises the exit section to allow the user to specify custom status codes and messages to correspond to those codes, implementing the API discussed in #25.

[codesections]

Merged master so that this PR will apply cleanly.

I also reverted deleting the mod.rs file. I believe this file isn't needed and had deleted it to test, but that's not logically related to this PR and I didn't mean to include the deletion as part of this PR.

[yoshuawuyts]

Overall LGTM! Few nits, but otherwise great!

[codesections]

Fixed those nits—agreed on both points.

[killercup]

Maybe I'm missing something but from a quick glance I think we can improve this design a bit :)

[killercup]

I'm confused. If you have a bunch of these and any one of them has this boolean flag set to true, you'll ignore all information and instead print a list of default statuses? I'd do this with an enum like enum ExitStatuses { DefaultStatuses, List(Vec<ExitStatus>) } (with ExitStatus containing fields for code and description)

[killercup]

You are already deriving Default which generates an equivalent method

[codesections]

Agreed on both points. I refactored the code and am much happier with how it reads now that the default value is set using an enum instead of with a sentential value. Thanks for the tip!

[killercup]

Nice! Sorry it took me a while to reply. I still don't know what the exact context for this is and how you'll use it, but one thing I'd definitely do is make a single method that allows you to get an iterator of exit status entries, for both cases. After a bit of fiddling I wrote this, that also moves the default list into a static (a constant value that will be part of the binary):

diff --git a/src/man.rs b/src/man.rs
index 53eda6c..177f0e9 100644
--- a/src/man.rs
+++ b/src/man.rs
@@ -19,33 +19,41 @@ pub struct Manual {
 
 #[derive(Debug, Clone)]
 enum ExitStatuses {
-  DefaultStatuses([ExitStatus; 3]),
+  DefaultStatuses,
   CustomStatuses(Vec<ExitStatus>),
 }
 
+static DEFAULT_STATUSES: [ExitStatus; 3] = [
+  ExitStatus {
+    code: Some(0),
+    description: Some("Successful program execution."),
+  },
+  ExitStatus {
+    code: Some(1),
+    description: Some("Unsuccessful program execution."),
+  },
+  ExitStatus {
+    code: Some(101),
+    description: Some("The program panicked."),
+  },
+];
+
 impl ExitStatuses {
   fn push(&mut self, new_status: ExitStatus) {
-    if let ExitStatuses::CustomStatuses(mut vec) = self.to_owned() {
+    if let ExitStatuses::CustomStatuses(vec) = self {
       vec.push(new_status);
-      *self = ExitStatuses::CustomStatuses(vec);
     }
   }
 
   fn set_to_default(&mut self) {
-    *self = ExitStatuses::DefaultStatuses([
-      ExitStatus {
-        code: Some(0),
-        description: Some("Successful program execution."),
-      },
-      ExitStatus {
-        code: Some(1),
-        description: Some("Unsuccessful program execution."),
-      },
-      ExitStatus {
-        code: Some(101),
-        description: Some("The program panicked."),
-      },
-    ]);
+    *self = ExitStatuses::DefaultStatuses;
+  }
+
+  fn iter(&self) -> impl Iterator<Item = &ExitStatus> {
+    match self {
+      ExitStatuses::CustomStatuses(vec) => vec.iter(),
+      ExitStatuses::DefaultStatuses => DEFAULT_STATUSES.iter(),
+    }
   }
 }
 
@@ -397,29 +405,19 @@ fn env(page: Roff, environment: &[Env]) -> Roff {
 /// ```
 fn exit_status(page: Roff, exit_statuses: &ExitStatuses) -> Roff {
   let mut arr = vec![];
-  match exit_statuses {
-    ExitStatuses::DefaultStatuses(default_statuses) => {
-      for status in default_statuses.iter() {
-        let code = format!("{}", status.code.expect("Set as part of default"));
-        let mut description = String::from(status.description.unwrap());
-        description.push_str("\n\n");
-        arr.push(list(&[bold(&code)], &[description]));
-      }
-    }
-    ExitStatuses::CustomStatuses(custom_statuses) => {
-      if custom_statuses.is_empty() {
-        return page;
-      }
-      for status in custom_statuses.iter() {
-        let code =
-          format!("{}", status.code.expect("Always set when not default"));
-        let mut description = String::from(status.description.unwrap_or(""));
-        description.push_str("\n\n");
-        arr.push(list(&[bold(&code)], &[description]));
-      }
-    }
+
+  for status in exit_statuses.iter() {
+    let code = format!("{}", status.code.unwrap());
+    let mut description = String::from(status.description.unwrap());
+    description.push_str("\n\n");
+    arr.push(list(&[bold(&code)], &[description]));
+  }
+
+  if !arr.is_empty() {
+    page.section("exit status", &arr)
+  } else {
+    page
   }
-  page.section("exit status", &arr)
 }
 
 /// Create a custom section.

I'm a bit unsure about the two unwraps in the for loop: It seems that both code and description are optional, but here they are required? What do you want to do? Make sure that all exit codes have code+description, or use fallback values here, or actually have this library crash your program when one of the options is None? (Not sure if this was a change in this PR, or if it needs to be addressed here, but I wanted to point it out.)

[codesections]

Nice! Sorry it took me a while to reply.

No need to apologize—I thought you replied very promptly so if this was you taking a while to reply, then I can't even imagine what you're like when you're fast :)

one thing I'd definitely do is make a single method that allows you to get an iterator of exit status entries, for both cases

I really like this change! I implemented it in the PR and will keep that pattern in mind for future code.

I'm a bit unsure about the two unwraps in the for loop

I think that the way we have it in the PR now correctly captures the semantics we want. code is None if the default method is called—which is how we can detect that the exit status was initialized with default rather than new. If the exit status was initialize with new, then it's safe to unwrap. (I changed it to expect to clarify this point).

The description is None whenever the user hasn't set a description—and keeping it as None instead of an empty string clarifies that there isn't a user-supplied description (and is consistent with how we treat other user-supplied optional text, like the help text for flags). But we don't want the program to crash if the user doesn't supply a description, so we substitute in a default value of '' at the last minute with an unwrap_or call.

[killercup]

Heh, "quick reply" to me means something like "it's still the same day for you" :)

I think this is a very interesting API decision here, so I have more nits. It's fine as-is but I wanted to let you know what I thought.

I think that the way we have it in the PR now correctly captures the semantics we want. code is None if the default method is called—which is how we can detect that the exit status was initialized with default rather than new. If the exit status was initialize with new, then it's safe to unwrap. (I changed it to expect to clarify this point).

It sounds like you want to have two variants of ExitStatus -- one that tells you to use the default set, and on that is an actual exit status. You should use an enum for this, too: This would let you express your intent more closely and also get rid of the possibility of accidentally doing ExitStatus::default().description("foo") -- because what does that even mean?

I'd probably do this differently. It's not wrong, but I think there is still one impedance mismatch: You call ExitStatus::default() to reset ExitStatuses. ExitStatus should not have that power/responsibility, I think. Off the top of my head I'd make the code field mandatory and add a method use_default_exit_statuses() to ExitStatuses.

[killercup]

Ah, so this line is what I was missing! I didn't get when you would ever set the exit status list to the default list – I assumed it was the set you were starting with and then when you add a custom one it gets overwritten.

This behavior here is surprising to me: When you call this public function, that has no docs, with a "special" value (code = None), it does an unusual thing. What's also weird: To get this special input you'd have to call default(), but from what I can tell this "special" mode should not be the "default".

[codesections]

Hmm, all of that is very interesting—I feel like I'm learning a lot from this conversation, so thanks for taking the time.

I'm not entirely sure what API you're suggesting when you said

You call ExitStatus::default() to reset ExitStatuses. ExitStatus should not have that power/responsibility, I think.

The current (pre-PR) API is that the user has no control over the exit status: exit statuses of 0, 1, and 101 are always printed, along with matching descriptions. As @yoshuawuyts and I discussed in #25, we think that most users will want to use those three statuses, so we'd like to keep the process of using them as concise/ergonomic as possible. The API we settled on in #25 was

let page = Manual::new("basic")
  .exit_status(ExitStatus::default())
  .render();

to display those three exit statuses and

let page = Manual::new("basic")
  .exit_status(
    ExitStatus::new(1)
    .description("Unsuccessful program execution.")
  )
  .exit_status(
    ExitStatus::new(3)
    .description("Invalid config.")
  )
  .render();

to display exit statuses different from those three. Are you suggesting a change from that API?

If we do want to change the API, we could do the following:

let page = Manual::new("basic")
  .exit_status(ExitStatus::with_default_values())
  .render();

to display those three exit statuses and

let page = Manual::new("basic")
  .exit_status(
    ExitStatus::with_code(1)
    .description("Unsuccessful program execution.")
  )
  .exit_status(
    ExitStatus::with_code(3)
    .description("Invalid config.")
  )
  .render();

However, that change wouldn't address your point when you said:

Off the top of my head I'd make the code field mandatory and add a method use_default_exit_statuses() to ExitStatuses

But (unless I'm misunderstanding) the ExitStatuses enum isn't public, so the user wouldn't be about to call the use_default_exit_statuses() method without other changes to the API.

(Whatever we decide, I agree that the API docs should be improved for this section)

[killercup]

Hmm, all of that is very interesting—I feel like I'm learning a lot from this conversation, so thanks for taking the time.

Cool! I'm happy to!

Are you suggesting a change from that API?

After reading #25: Yes, I disagree with that API.

My main issue is that the .exit_status method as proposed there (and here) has two modes of operation: Adding one exit status to the list of exit statuses (this makes sense to me), and replacing the currently set exit statuses with three default ones. This latter case feels very weird to me, which made me suggest to add a new method, use_default_exit_statuses, next to .exit_status on Man.

Let me write a comment on #25 with a summary of the API I'd like to suggest.

[codesections]

I like the API you proposed in #25. I added a slight amendment inspired by how clap-rs handles a similar issue and provided details in that thread.