Use different crate for argument parsing

[d-e-s-o]

The author of argparse has made it fairly clear that the crate is in maintenance mode (at best; at worst I'd say it's abandoned). While it does pretty much what we want it to, there are flaws that I find increasingly annoying:

1. We had to implement some workaround for a bug in its code that won't get fixed because the proposed fix broke backwards compatibility.
2. The help texts it builds and displays are extremely rigid in their layout, providing a sub-par user experience (I've always dislike how we list all available commands/subcommand only on a single line) and not playing nice with extensions, of which there could be a lot.
3. Let's face it, the way it is used is awkward and definitely not as obvious as it could be. While our code base has evolved around it fairly nicely, I still find myself trying to understand nuances related to argument handling over and over again.

So at this point I would be interested in considering alternatives. The only viable one I am aware of is structopt. In the past we've shied away from it due to bloat (without having conducted actual testing on this crate), and while that is still a concern, I'd say point 2) above tipped me over as user experience over all is still higher ranked.

We should start by prototyping use of structopt and having a look at the outcome (code structure and resulting binary size).

[d-e-s-o]

Not to forget that bash completion should come much easier when we use this crate.

[robinkrahl]

Please have a look at my clap branch which contains a first implementation using structopt. There are some issues with it:

• We still have to implement the error handling (i. e. choose the correct input and output streams, determine the return value). Currently I just call clap::Error::exit (see args.rs:handle_arguments).
• Currently, the available options for an argument are not displayed in the help message. We might have to do that manually.

But the code is a lot shorter and a lot more readable – and it should get even better if we improve the Command! macro (→ no type annotation in the closure, no need for empty structs).

Here are some measurements of the code and binary size:

$ git checkout -q devel && wc nitrocli/src/arg*.rs
  984  3205 28739 nitrocli/src/args.rs
  158   562  4498 nitrocli/src/arg_util.rs
 1142  3767 33237 total
$ git checkout -q clap && wc nitrocli/src/arg*.rs
  540  1872 16250 nitrocli/src/args.rs
  133   434  3674 nitrocli/src/arg_util.rs
  673  2306 19924 total
$ git checkout -q devel && cd nitrocli && cargo build -q --release >/dev/null 2>&1 && ls -l target/release/nitrocli && cd ..
-rwxr-xr-x 2 robin robin 2362648 Jan  7 16:54 target/release/nitrocli
$ git checkout -q clap && cd nitrocli && cargo build -q --release >/dev/null 2>&1 && ls -l target/release/nitrocli && cd ..
-rwxr-xr-x 2 robin robin 2735024 Jan  7 16:55 target/release/nitrocli

I think that’s acceptable.

[d-e-s-o]

Thanks for working on this issue! Indeed, it looks nicer. Yes, I agree the size increase is acceptable.

Not a full review (I gather you are not done yet), but few preliminary comments:

• TODO: tell structopt about stdout and stderr according to ctx: dumb question, but why does it need to know that? It appears that errors are returned as objects (which we then have control over printing) and stdout it shouldn't have to use (?)
• would now be a good time to switch away from using 3rd person in help texts (and perhaps comments)?
  • e.g., "Opens a hidden volume on a Nitrokey Storage" -> "Open a hidden volume on a Nitrokey Storage"? I suspect you prefer the former, though. Is it more common? I find it somewhat awkward, but perhaps I just don't pay enough attention...
  • we also seem to be mixing styles and should unify one way or another (e.g., "Set the capslock option to the given HOTP slot")
• TODO: do not exit; return exit code instead: Is there a complication or have you just not gotten around doing that?
  • I have been considering using anyhow for error handling. I've used it elsewhere and overall it was an improvement.
    In a nutshell, we could do away with our Error type and use anyhow::Error everywhere. It means few more allocations but that shouldn't be a concern for this program. It allows for easily annotating errors with contextual information and it can report error chains (via std::error::Error's source method; which btw., I am not sure nitrokey implements correctly, it should have an implementation that forwards the call to the inner error, but I doubt it does that [it's something I wanted to do, but haven't gotten around yet]).
    The devil may be in the detail, though, as I don't want to have to cast errors and I am not sure we need to/can get around that (in tests it's fine)

But the code is a lot shorter and a lot more readable – and it should get even better if we improve the Command! macro (→ no type annotation in the closure, no need for empty structs).

Indeed, that would be awesome! Let me know if you need help with that.

[robinkrahl]

TODO: tell structopt about stdout and stderr according to ctx: dumb question, but why does it need to know that? It appears that errors are returned as objects (which we then have control over printing) and stdout it shouldn't have to use (?)

Yes, I wrote that comment when I deleted the previous std* code and was not yet aware of structopt’s behavior. It turns out that, contrary to argparse, clap does not try to write anything if we call the *_safe functions, so it’s easy to fix this.

would now be a good time to switch away from using 3rd person in help texts (and perhaps comments)?

I tried to consistently use third person because that’s what structopt does for the auto-generated commands and options. Personally, I have a slight preference for imperatives.

TODO: do not exit; return exit code instead: Is there a complication or have you just not gotten around doing that?

No complications, similar to the other TODO comment.

I have been considering using anyhow for error handling. I've used it elsewhere and overall it was an improvement.

I haven’t used anyhow yet, but I think we should discuss this separately. Regarding nitrokey’s source implementation, could you be more specific? At first glance, I don’t see the problem.

Indeed, that would be awesome! Let me know if you need help with that.

If you have an easy solution to get this code to work, that would be nice:

Command! {OtpCommand, [
  /// Prints the Nitrokey configuration
  Get => |ctx| commands::config_get(ctx),
  /// Changes the Nitrokey configuration
  Set(ConfigSetArgs) => config_set,
]}

[robinkrahl]

I fixed the two issues mentioned earlier (enum variants in help text; exit handling), squashed some commits, added more explanations to the commit messages and opened a pull request (#97) for in-depth discussion of the implementation.

[d-e-s-o]

I have been considering using anyhow for error handling. I've used it elsewhere and overall it was an improvement.

I haven’t used anyhow yet, but I think we should discuss this separately.

Yes, different issue. I just brought it up because it may help in the transition (no clap::Error variant needed; not saying it will be needed, but there is a possibility it may).

[Indeed, checking the latest revision you did add it]

Regarding nitrokey’s source implementation, could you be more specific? At first glance, I don’t see the problem.

What I was trying to say is that in an enum style error type (as nitrocli and nitrokey-rs both employ), a naive implementation of std::error::Error will have the default implementation for source, which will not be forwarded to the individual variants. That means that an error chain would be broken and inner errors not intuitively reachable as they should be.
I did check now, though, and it seems all is well.

impl error::Error for Error {
    fn source(&self) -> Option<&(dyn error::Error + 'static)> {
        match *self {
            Error::CommandError(ref err) => Some(err),
            Error::CommunicationError(ref err) => Some(err),
            Error::ConcurrentAccessError => None,
            Error::LibraryError(ref err) => Some(err),
            Error::PoisonError(ref err) => Some(err),
            Error::RandError(ref err) => Some(err.as_ref()),
            Error::UnexpectedError => None,
            Error::UnknownError(_) => None,
            Error::Utf8Error(ref err) => Some(err),
        }
    }
}

Indeed, that would be awesome! Let me know if you need help with that.

If you have an easy solution to get this code to work, that would be nice:

Command! {OtpCommand, [
  /// Prints the Nitrokey configuration
  Get => |ctx| commands::config_get(ctx),
  /// Changes the Nitrokey configuration
  Set(ConfigSetArgs) => config_set,
]}

That...was...intense o_O But should work with the following patch:

--- nitrocli/src/arg_util.rs
+++ nitrocli/src/arg_util.rs
@@ -24,12 +24,18 @@ macro_rules! count {
   }
 }

+/// Translate an optional source into an optional destination.
+macro_rules! tr {
+  ($dst:tt, $src:tt) => { $dst };
+  ($dst:tt) => { };
+}
+
 macro_rules! Command {
-  ( $name:ident, [ $( $var:ident($inner:ident) => $exec:expr, ) *] ) => {
+  ( $name:ident, [ $( $var:ident$(($inner:ty))? => $exec:expr, ) *] ) => {
     #[derive(Debug, PartialEq, structopt::StructOpt)]
     pub enum $name {
       $(
-        $var($inner),
+        $var$(($inner))?,
       )*
     }

@@ -41,7 +47,7 @@ macro_rules! Command {
       ) -> crate::Result<()> {
         match self {
           $(
-            $name::$var(args) => $exec(ctx, args),
+            $name::$var$((tr!(args, $inner)))? => $exec(ctx $(,tr!(args, $inner))?),
           )*
         }
       }
diff --git nitrocli/src/args.rs nitrocli/src/args.rs
index a70e816..694e1b 100644
--- nitrocli/src/args.rs
+++ nitrocli/src/args.rs
@@ -130,10 +130,6 @@ pub struct ConfigArgs {
   subcmd: ConfigCommand,
 }

-/// Prints the Nitrokey configuration
-#[derive(Debug, PartialEq, structopt::StructOpt)]
-pub struct ConfigGetArgs {}
-
 /// Changes the Nitrokey configuration
 #[derive(Debug, PartialEq, structopt::StructOpt)]
 pub struct ConfigSetArgs {
@@ -374,7 +370,7 @@ pub struct UnencryptedSetArgs {
 }

 Command! {ConfigCommand, [
-  Get(ConfigGetArgs) => |ctx, _| commands::config_get(ctx),
+  Get => commands::config_get,
   Set(ConfigSetArgs) => config_set,
 ]}

I fixed the two issues mentioned earlier (enum variants in help text; exit handling), squashed some commits, added more explanations to the commit messages and opened a pull request (#97) for in-depth discussion of the implementation.

Thanks! I'll have a look tomorrow, I hope.

[d-e-s-o]

would now be a good time to switch away from using 3rd person in help texts (and perhaps comments)?

I tried to consistently use third person because that’s what structopt does for the auto-generated commands and options. Personally, I have a slight preference for imperatives.

That's only for the auto generated -h/--help and -v/--verbose? Or did I miss something? I'd suggest we can ignore those. Let's go with the imperative versions instead, as we both prefer them.

[robinkrahl]

That's only for the auto generated -h/--help and -v/--verbose?

Yes, but as this is the only place where the messages are used, I think it’s even more important to optimize them for this output (i. e. make them consistent).

[d-e-s-o]

What I meant is: structopt's third person is only visible at two locations:

FLAGS:
    -h, --help       Prints help information              <------ #1
    -V, --version    Prints version information           <------ #2

The rest we have full control over. Correct? I don't want two sentences emitted by a random crate dictate how we formulate the rest of our help texts.

[d-e-s-o]

Are you okay with me changing it? Then I'll adjust your changes and then merge. The rest looks great to me.

[robinkrahl]

The rest we have full control over. Correct?

Basically yes, there’s also a help subcommand.

Are you okay with me changing it?

Yes, if you prefer that then go ahead.

[d-e-s-o]

I was under the impression that you also preferred that.

I tried to consistently use third person because that’s what structopt does for the auto-generated commands and options. Personally, I have a slight preference for imperatives.

Or did I misunderstand? My worry is that since we both write in that style usually, there will be messages that end up in it. So from that perspective going with what structopt/clap does just because it does that seems like the worse choice. I don't feel super strongly, so if you disagree we'll just keep what you have.

[robinkrahl]

Or did I misunderstand? My worry is that since we both write in that style usually, there will be messages that end up in it. So from that perspective going with what `structopt`/`clap` does just because it does that seems like the worse choice. I don't feel super strongly, so if you disagree we'll just keep what you have.

Well, I always have to check the existing code anyway if I haven’t worked on a project recently. For the nitrokey crate, for example, I try to use third person in the doc comments as it is recommended by the Rust style guidelines [0]. Anyway, I don’t really care as long as we are consistent. [0] https://doc.rust-lang.org/1.0.0/style/style/comments.html#sentence-structure

[d-e-s-o]

I'll leave it as-is. Let's see how it goes.

[d-e-s-o]

Merged #97. Let's give this some bake time before doing a new release.

Would you mind sharing your findings around the shell completion, @robinkrahl ? You mentioned there were complications.
Would be interesting to hear as I have no experience setting this up with clap (and only read through the instructions ages ago when we were investigating viable options).

[robinkrahl]

So far I only looked at the documentation. In their example, they use the include! macro to embed the app definition into build.rs. Most likely this will not work for us due to the local imports. So I imagine getting the relevant types into build.rs would be rather cumbersome. Generating the completions at run time (as a new subcommand) should be trivial but less elegant.

[d-e-s-o]

Ah, thanks. It seems as if no include! should be needed if we have a way to get the App (if I understand the intention correctly). Perhaps we can factor out functionality into a library crate and just "use" that from build.rs. I don't recall if that's possible. Anyway, it's not primarily what this issue is about. Let me close it accordingly. If you intend to work on this functionality, mind updating the task list (I've added an item for that)?

[d-e-s-o]

Forgot that we already have an issue for shell completion support: #57