Argument parsing: Switch from docopt to clap

[dbrgn]

Fixes #106.

[pksunkara]

You should also be able to generate completions files using https://github.com/clap-rs/clap/blob/master/clap_generate/src/lib.rs

[dbrgn]

I'll work on this again when clap v3 is released. We cannot release to crates.io if we depend on unreleased crates. And it seems that clap v3 still has quite some work ahead: https://github.com/clap-rs/clap/milestone/76 The work so far looks promising though!

[pksunkara]

We released a beta on crates.io. But yes, we still have some work to finish.

[dbrgn]

I wonder if standalone flags (like --show-paths) should be changed to subcommands instead... Might be cleaner, but might be a breaking change for some of them.

[niklasmohrin]

@dbrgn I don't think we should do that. If I recall correctly the spec requires --update to be a flag and the subcommands could clash with actual pages (maybe someone writes a program called show-paths that gets its own page)

[dbrgn]

Hah, that's a good point. I'm convinced 🙂

[dbrgn]

Still open:

• Spec compliance
• Auto-generate completion files

However, these things can be handled in separate issues. I think it would be good if we could merge this soon, the current version should have feature-parity with master/docopt.

Current status regarding binary size:

• main at 3f49100: 8483 KiB (unstripped), 5355 KiB (stripped)
• clap: 7581 KiB (unstripped), 4519 KiB (stripped)

...so we shaved off almost a megabyte.

@niklasmohrin could you help testing this, to see if there's any regression? I hope I didn't overlook anything 🙂

[dbrgn]

Dependency count increased (153 vs previously 140), but that isn't necessarily bad. Compilation time for a clean release build went down slightly on my 12-core machine (48.9s now vs 53.1s previously).

[dbrgn]

I also took a look in callgrind (see #106 (comment) for comparison).

Assembly instruction count for tldr::main went down from 9'214'656 to 515'050 😄 Nice!

Hyperfine benchmark comparison on my machine:

# docopt
Benchmark #1: target/release/tldr tar
  Time (mean ± σ):      15.9 ms ±   0.7 ms    [User: 1.8 ms, System: 2.7 ms]
  Range (min … max):    13.2 ms …  17.7 ms    150 runs

clap
Benchmark #1: target/release/tldr tar
  Time (mean ± σ):      12.4 ms ±   0.5 ms    [User: 1.0 ms, System: 2.2 ms]
  Range (min … max):    10.2 ms …  13.5 ms    150 runs

Hah, that's an awesome improvement! 🎉 With this, we clearly beat fast-tldr-gh and are now the second fastest implementation in the benchmark. Only the Zig based implementation is faster, but it doesn't support user configuratoin at all, so it's an unfair comparison.

[niklasmohrin]

Very very nice. 👍

It looks like everything works as expected, all integration tests are passing so there shouldn't be a problem. I left some comments that I would like to see addressed / discussed before merging though.

Clippy has some suggestions that I agree with, those would be good to include.

I think we should first merge this and then update the benchmarks, this way one can check for the commit on master on the specified date. Benchmark and change can be separated, so we should do so

[niklasmohrin]

On first look, I didn't like the colors. I think they do improve readability though. I did a quick check and tools like bat also have colored help, although bat supports different color schemes for its main output.

I think we can keep the colors :)

Change requests addressed

[niklasmohrin]

I just noticed that the examples are not in the --help anymore, is this planned?

[niklasmohrin]

Great, only the two clippy lints that CI annotated and then everything should be ready to ship! 🚀

[dbrgn]

Regarding the colors: I'm not really sure if it's an improvement in readability, or just a distraction. I myself am not a big fan of them for that reason. I asked two people what they think about it, and both said "hm, whether or not that looks good depends on the color scheme of the terminal", and both said that they don't think colors are that important for help texts. (For the error messages, I think the colors are useful!)

There's another reason why colored help may be a bad idea: We offer controlling the page output colors through our config file. Some users might want to make help text colors configurable as well. I don't want to implement such a feature, I think it's unneeded. And if someone would want to turn off colors for the help text by setting NO_COLOR=1, that would disable tldr colors as well (which is probably not intended).

I think I'll try to disable help colors again.

Regarding examples, you are right, those should be added back.

[niklasmohrin]

🤷‍♂️ Sure, colors are not a must-have.

[dbrgn]

I just noticed that the examples are not in the --help anymore, is this planned?

When researching on how to implement this, I stumbled upon my own question 😂 clap-rs/clap#2477

[niklasmohrin]

"What, no, this PR is not that old really" 😆

On a side note, the examples could be in their own file so that we could (later) add an integration test that reads the file and ensures the examples work as expected

[dbrgn]

Yeah, I'm a bit unsure about the examples section. In essence, this is a tldr page, but worse.

Maybe we could simply redirect users to tldr tldr instead`?

[dbrgn]

Here's an implementation of that suggestion:

@niklasmohrin do you like it? Or do you think the examples should still be provided? The tldr page only contains the shared features according to the client spec of course, so it will always be a subset of the functionality.

Another approach would be linking to https://dbrgn.github.io/tealdeer/. That might actually be the best solution, because we have much more possibilities to structure the content to make it easier to read / understand.

I think docs are valuable, but I'm not sure it should be part of the --help text.

Edit: I think this is the best solution:

[niklasmohrin]

Yep, that seems appropriate. We need to add these examples explicitly in the docs though

[dbrgn]

Yep, I'll create a separate issue for improved docs.

@niklasmohrin I also added a commit that adds you to the authors in Cargo.toml and thus in the --help output. OK for you?

If yes, I can merge this PR now.

[dbrgn]

🎉 finally!