cli: Fix clap3 failures/warnings/regressions

* The API changes are a mixed bag but look good overall
* Had to swap the failure exit codes (as clap now `exit(1)` instead of 2)
  Keeping the old codes is possible, but doesn't seem worth the added complexity
  See clap-rs/clap#3426
* Stumbled upon one shell completion regression, so pining an old clap_complete version for now
  See clap-rs/clap#3697
* Subcommand completion no longer erroneously include `-V` and `--version`
* Help and diagnostic outputs seem a bit nicer

Author: vincentdephily

Cargo.lock

@@ -19,8 +19,9 @@ exclude = ["/benches/*.log"]
 ansi_term = "0.12.1"
 anyhow = "1.0.32"
 atty = "0.2.14"
-clap = {version = "3.1.15", features = ["cargo"]}
-clap_complete = "3.1.3"
+clap = { version = "3.1.15", default_features = false, features = ["cargo", "color", "suggestions"] }
+# Waiting on https://github.com/clap-rs/clap/issues/3697 to relax version
+clap_complete = "=3.1.0"
 crossbeam-channel = "0.5.0"
 env_logger = {version = "0.9.0", default_features = false, features = ["termcolor", "atty"]}
 log = "0.4.11"

Cargo.toml

@@ -1,75 +1,73 @@
-use clap::{crate_version, App, AppSettings, Arg, SubCommand};
+use clap::{crate_version, AppSettings, Arg, Command};
 
 /// Generate cli argument parser without the `complete` subcommand.
-pub fn build_cli_nocomplete() -> App<'static> {
+pub fn build_cli_nocomplete() -> Command<'static> {
     let arg_limit =
-        Arg::with_name("limit").long("limit")
-                               .takes_value(true)
-                               .default_value("10")
-                               .help("Use the last N merge times to predict next merge time.");
+        Arg::new("limit").long("limit")
+                         .takes_value(true)
+                         .default_value("10")
+                         .help("Use the last N merge times to predict next merge time.");
     let arg_pkg =
-        Arg::with_name("package").takes_value(true).help("Show only packages matching <package>.");
-    let arg_exact =
-        Arg::with_name("exact").short('e')
-                               .long("exact")
-                               .help("Match package with a string instead of a regex.")
-                               .long_help(
-                                          "Match package with a string instead of a regex. \
+        Arg::new("package").takes_value(true).help("Show only packages matching <package>.");
+    let arg_exact = Arg::new("exact").short('e')
+                                     .long("exact")
+                                     .help("Match package with a string instead of a regex.")
+                                     .long_help(
+                                                "Match package with a string instead of a regex. \
                     Regex is case-insensitive and matches on category/name (see \
                     https://docs.rs/regex/*/regex/#syntax). String is case-sentitive and \
                     matches on whole name, or whole category/name if it contains a /.",
+    );
+    let arg_show_l =
+        Arg::new("show").short('s')
+                        .long("show")
+                        .value_name("h,m,u,s,a")
+                        .validator(|s| find_invalid("hmusa", &s))
+                        .default_value("m")
+                        .help("Show (h)eaders, (m)erges, (u)nmerges, (s)yncs, and/or (a)ll.")
+                        .long_help(
+                                   "Show individual (m)erges, (u)nmerges, portage tree (s)yncs, \
+                    or (a)ll of these (any letters combination).",
         );
-    let arg_show_l = Arg::with_name("show")
-        .short('s')
-        .long("show")
-        .value_name("h,m,u,s,a")
-        .validator(|s| find_invalid("hmusa", &s))
-        .default_value("m")
-        .help("Show (h)eaders, (m)erges, (u)nmerges, (s)yncs, and/or (a)ll.")
-        .long_help("Show individual (m)erges, (u)nmerges, portage tree (s)yncs, \
-                    or (a)ll of these (any letters combination).");
-    let arg_show_s = Arg::with_name("show")
-        .short('s')
-        .long("show")
-        .value_name("h,p,t,s,a")
-        .validator(|s| find_invalid("hptsa", &s))
-        .default_value("p")
-        .help("Show (h)eaders, (p)ackages, (t)otals, (s)yncs, and/or (a)ll.")
-        .long_help("Show per-(p)ackage merges/unmerges, (t)otal merges/unmerges, \
-                    portage tree (s)yncs, or (a)ll of these (any letters combination).");
-    let arg_group =
-        Arg::with_name("group").short('g')
-                               .long("groupby")
-                               .value_name("y,m,w,d")
-                               .possible_values(&["y", "m", "w", "d"])
-                               .hide_possible_values(true)
-                               .help("Group by (y)ear, (m)onth, (w)eek, or (d)ay.")
-                               .long_help(
-                                          "Group by (y)ear, (m)onth, (w)eek, or (d)ay.\n\
+    let arg_show_s =
+        Arg::new("show").short('s')
+                        .long("show")
+                        .value_name("h,p,t,s,a")
+                        .validator(|s| find_invalid("hptsa", &s))
+                        .default_value("p")
+                        .help("Show (h)eaders, (p)ackages, (t)otals, (s)yncs, and/or (a)ll.")
+                        .long_help(
+                                   "Show per-(p)ackage merges/unmerges, (t)otal merges/unmerges, \
+                    portage tree (s)yncs, or (a)ll of these (any letters combination).",
+        );
+    let arg_group = Arg::new("group").short('g')
+                                     .long("groupby")
+                                     .value_name("y,m,w,d")
+                                     .possible_values(&["y", "m", "w", "d"])
+                                     .hide_possible_values(true)
+                                     .help("Group by (y)ear, (m)onth, (w)eek, or (d)ay.")
+                                     .long_help(
+                                                "Group by (y)ear, (m)onth, (w)eek, or (d)ay.\n\
                     The grouping key is displayed in the first column. \
                     Weeks start on monday and are formated as 'year-weeknumber'.",
-        );
-    App::new("emlop")
+    );
+    Command::new("emlop")
         .version(crate_version!())
-        .global_setting(AppSettings::ColoredHelp)
         .global_setting(AppSettings::DeriveDisplayOrder)
-        .global_setting(AppSettings::UnifiedHelpMessage)
-        .setting(AppSettings::DisableHelpSubcommand)
-        .setting(AppSettings::InferSubcommands)
-        .setting(AppSettings::SubcommandRequiredElseHelp)
-        // FIXME clap3 what's the equivalent ? .setting(AppSettings::VersionlessSubcommands)
+        .disable_help_subcommand(true)
+        .infer_subcommands(true)
+        .arg_required_else_help(true)
         .about("A fast, accurate, ergonomic EMerge LOg Parser.\n\
                 https://github.com/vincentdephily/emlop")
         .after_help("Subcommands can be abbreviated down to a single letter.\n\
                      Subcommands have their own -h / --help.\n\
-                     Exit code is 0 if sucessful, 1 in case of errors (bad argument...), \
-                     2 if search found nothing.")
-        .help_message("Show short (-h) or detailed (--help) help.")
-        .arg(Arg::with_name("utc")
+                     Exit code is 0 if sucessful, 1 if search found nothing, 2 in case of argument errors.")
+        .mut_arg("help", |a| a.help("Show short (-h) or detailed (--help) help."))
+        .arg(Arg::new("utc")
              .long("utc")
              .global(true)
              .help("Parse/display dates in UTC instead of local time"))
-        .arg(Arg::with_name("from")
+        .arg(Arg::new("from")
              .value_name("date")
              .short('f')
              .long("from")
@@ -79,7 +77,7 @@ pub fn build_cli_nocomplete() -> App<'static> {
              .long_help("Only parse log entries after <date>.\n\
                          Accepts formats like '2018-03-04', '2018-03-04 12:34:56', \
                          '2018-03-04T12:34', '1 year, 2 months', '10d', and unix timestamps."))
-        .arg(Arg::with_name("to")
+        .arg(Arg::new("to")
              .value_name("date")
              .short('t')
              .long("to")
@@ -89,7 +87,7 @@ pub fn build_cli_nocomplete() -> App<'static> {
              .long_help("Only parse log entries before <date>.\n\
                          Accepts formats like '2018-03-04', '2018-03-04 12:34:56', \
                          '2018-03-04T12:34', '1 year, 2 months', '10d', and unix timestamps."))
-        .arg(Arg::with_name("duration")
+        .arg(Arg::new("duration")
              .value_name("format")
              .long("duration")
              .global(true)
@@ -103,7 +101,7 @@ pub fn build_cli_nocomplete() -> App<'static> {
     s:                           630
     human:    10 minutes, 30 seconds
 "))
-        .arg(Arg::with_name("date")
+        .arg(Arg::new("date")
              .value_name("format")
              .long("date")
              .global(true)
@@ -120,24 +118,24 @@ pub fn build_cli_nocomplete() -> App<'static> {
     compact:      20220131085946
     unix:         1643619586
 "))
-        .arg(Arg::with_name("logfile")
+        .arg(Arg::new("logfile")
              .value_name("file")
              .long("logfile")
              .short('F')
              .global(true)
              .takes_value(true)
              .default_value("/var/log/emerge.log")
              .help("Location of emerge log file."))
-        .arg(Arg::with_name("verbose")
+        .arg(Arg::new("verbose")
              .short('v')
              .global(true)
-             .multiple(true)
+             .multiple_occurrences(true)
              .help("Increase verbosity (can be given multiple times).")
              .long_help("Increase verbosity (defaults to errors only)
     -v:      show warnings
     -vv:     show info
     -vvv:    show debug"))
-        .arg(Arg::with_name("color")
+        .arg(Arg::new("color")
              .long("color").alias("colour")
              .global(true)
              .takes_value(true)
@@ -146,23 +144,21 @@ pub fn build_cli_nocomplete() -> App<'static> {
              .default_value("auto")
              .value_name("when")
              .help("Enable color (auto/always/never/y/n)."))
-        .subcommand(SubCommand::with_name("log")
+        .subcommand(Command::new("log")
                     .about("Show log of sucessful merges, unmerges and syncs.")
                     .long_about("Show log of sucessful merges, unmerges and syncs.\n\
 * (Un)merges: date, duration, package name-version.\n\
 * Syncs:      date, duration.")
-                    .help_message("Show short (-h) or detailed (--help) help.")
                     .arg(&arg_show_l)
                     .arg(&arg_exact)
                     .arg(&arg_pkg))
-        .subcommand(SubCommand::with_name("predict")
+        .subcommand(Command::new("predict")
                     .about("Predict merge time for current or pretended merges.")
                     .long_about("Predict merge time for current or pretended merges.\n\
 * If input is a terminal, predict time for the current merge (if any).\n\
 * If input is a pipe (for example by running `emerge -rOp|emlop p`), predict time for those merges.")
-                    .help_message("Show short (-h) or detailed (--help) help.")
                     .arg(&arg_limit))
-        .subcommand(SubCommand::with_name("stats")
+        .subcommand(Command::new("stats")
                     .about("Show statistics about sucessful merges, unmerges and syncs.")
                     .long_about("Show statistics about sucessful (un)merges (overall or per \
                                  package) and syncs.\n\
@@ -171,7 +167,6 @@ pub fn build_cli_nocomplete() -> App<'static> {
                                  * Total:     merge count, total merge time, average merge time,   \
                                  unmerge count, total unmerge time, average unmerge time.\n\
                                  * Sync:      sync count,  total sync time,  predicted sync time.")
-                    .help_message("Show short (-h) or detailed (--help) help.")
                     .arg(&arg_show_s)
                     .arg(&arg_group)
                     .arg(&arg_exact)
@@ -180,19 +175,23 @@ pub fn build_cli_nocomplete() -> App<'static> {
 }
 
 /// Generate cli argument parser.
-pub fn build_cli() -> App<'static> {
+pub fn build_cli() -> Command<'static> {
     let c = build_cli_nocomplete();
-    c.subcommand(SubCommand::with_name("complete")
-                 .about("Generate shell completion script.")
-                 .long_about("Write shell completion script to stdout.\n\n\
+    c.subcommand(
+                 Command::new("complete").about("Generate shell completion script.")
+                                         .long_about(
+        "Write shell completion script to stdout.\n\n\
 You should redirect the output to a file that will be sourced by your shell.\n\
 For example: `emlop complete bash > ~/.bash_completion.d/emlop`.\n\
 To apply the changes, either restart you shell or `source` the generated file.
-")
-                 .arg(Arg::with_name("shell")
-                      .help("Target shell")
-                      .required(true)
-                      .possible_values(&["bash","zsh","fish"])))
+",
+    )
+                                         .arg(
+        Arg::new("shell").help("Target shell")
+                         .required(true)
+                         .possible_values(&["bash", "zsh", "fish"]),
+    ),
+    )
 }
 
 /// Clap validation helper that checks that all chars are valid.

src/cli.rs

@@ -7,7 +7,7 @@ use std::{collections::{BTreeMap, HashMap},
 ///
 /// We store the start times in a hashmap to compute/print the duration when we reach a stop event.
 pub fn cmd_list(args: &ArgMatches, subargs: &ArgMatches, st: &Styles) -> Result<bool, Error> {
-    let show = value_t!(subargs, "show", Show).unwrap();
+    let show = subargs.value_of_t("show").unwrap();
     let hist = new_hist(args.value_of("logfile").unwrap().into(),
                         value_opt(args, "from", parse_date, st.date_offset),
                         value_opt(args, "to", parse_date, st.date_offset),
@@ -101,7 +101,7 @@ impl Times {
 /// First loop is like cmd_list but we store the merge time for each ebuild instead of printing it.
 /// Then we compute the stats per ebuild, and print that.
 pub fn cmd_stats(args: &ArgMatches, subargs: &ArgMatches, st: &Styles) -> Result<bool, Error> {
-    let show = value_t!(subargs, "show", Show).unwrap();
+    let show = subargs.value_of_t("show").unwrap();
     let timespan_opt = value_opt(subargs, "group", parse_timespan, ());
     let hist = new_hist(args.value_of("logfile").unwrap().into(),
                         value_opt(args, "from", parse_date, st.date_offset),
@@ -552,7 +552,7 @@ mod tests {
     fn predict_tty() {
         emlop().args(&["p", "-F", "test/emerge.10000.log"])
                .assert()
-               .code(2)
+               .code(1)
                .stdout("No pretended merge found\n");
     }
 
@@ -562,7 +562,7 @@ mod tests {
     fn predict_emerge_p() {
         let _cache_cargo_build = emlop();
         let t = vec![// Check garbage input
-                     ("blah blah\n", format!("No pretended merge found\n"), 2),
+                     ("blah blah\n", format!("No pretended merge found\n"), 1),
                      // Check all-unknowns
                      ("[ebuild   R   ~] dev-lang/unknown-1.42\n",
                       format!("dev-lang/unknown-1.42                         ? \n\
@@ -624,7 +624,7 @@ mod tests {
              0),
             (&["-F","test/emerge.10000.log","s","--from","2018-02-03T23:11:47","--to","2018-02-04","notfound","-sa"],
              "",
-             2),
+             1),
         ];
         for (a, o, e) in t {
             emlop().args(a).assert().code(e).stdout(o);
@@ -835,34 +835,34 @@ mod tests {
     #[test]
     fn exit_status() {
         // 0: no problem
-        // 1: user or program error
-        // 2: command ran properly but didn't find anything
+        // 1: command ran properly but didn't find anything
+        // 2: user or program error
         let t: Vec<(&[&str], i32)> =
             vec![// Help, version, badarg (clap)
                  (&["-h"], 0),
                  (&["-V"], 0),
                  (&["l", "-h"], 0),
-                 (&[], 1),
-                 (&["s", "--foo"], 1),
-                 (&["badcmd"], 1),
+                 (&[], 2),
+                 (&["s", "--foo"], 2),
+                 (&["badcmd"], 2),
                  // Bad arguments (emlop)
-                 (&["l", "--logfile", "notfound"], 1),
-                 (&["s", "--logfile", "notfound"], 1),
-                 (&["p", "--logfile", "notfound"], 1),
-                 (&["l", "bad regex [a-z"], 1),
-                 (&["s", "bad regex [a-z"], 1),
-                 (&["p", "bad regex [a-z"], 1),
+                 (&["l", "--logfile", "notfound"], 2),
+                 (&["s", "--logfile", "notfound"], 2),
+                 (&["p", "--logfile", "notfound"], 2),
+                 (&["l", "bad regex [a-z"], 2),
+                 (&["s", "bad regex [a-z"], 2),
+                 (&["p", "bad regex [a-z"], 2),
                  // Normal behaviour
-                 (&["-F", "test/emerge.10000.log", "p"], 2),
+                 (&["-F", "test/emerge.10000.log", "p"], 1),
                  (&["-F", "test/emerge.10000.log", "l"], 0),
-                 (&["-F", "test/emerge.10000.log", "l", "-s"], 0),
+                 (&["-F", "test/emerge.10000.log", "l", "-sm"], 0),
                  (&["-F", "test/emerge.10000.log", "l", "-e", "icu"], 0),
-                 (&["-F", "test/emerge.10000.log", "l", "-e", "unknown"], 2),
-                 (&["-F", "test/emerge.10000.log", "l", "--from", "2018-09-28"], 2),
-                 (&["-F", "test/emerge.10000.log", "l", "-s", "--from", "2018-09-28"], 2),
+                 (&["-F", "test/emerge.10000.log", "l", "-e", "unknown"], 1),
+                 (&["-F", "test/emerge.10000.log", "l", "--from", "2018-09-28"], 1),
+                 (&["-F", "test/emerge.10000.log", "l", "-sm", "--from", "2018-09-28"], 1),
                  (&["-F", "test/emerge.10000.log", "s"], 0),
                  (&["-F", "test/emerge.10000.log", "s", "-e", "icu"], 0),
-                 (&["-F", "test/emerge.10000.log", "s", "-e", "unknown"], 2),];
+                 (&["-F", "test/emerge.10000.log", "s", "-e", "unknown"], 1),];
         for (a, e) in t {
             emlop().args(a).assert().code(e);
         }