Refactor CLI flag parsing (#2025)

1 files changed, 211 insertions, 135 deletions

diff --git a/cli/flags.rs b/cli/flags.rs
index 9d187ecac..cf1bb72bc 100644
--- a/cli/flags.rs
+++ b/cli/flags.rs
@@ -1,7 +1,6 @@
 // Copyright 2018-2019 the Deno authors. All rights reserved. MIT license.
+use clap::{App, AppSettings, Arg, ArgMatches, SubCommand};
 use deno::v8_set_flags;
-use getopts;
-use getopts::Options;
 
 // Creates vector of strings, Vec<String>
 #[cfg(test)]
@@ -12,7 +11,6 @@ macro_rules! svec {
 #[cfg_attr(feature = "cargo-clippy", allow(stutter))]
 #[derive(Clone, Debug, PartialEq, Default)]
 pub struct DenoFlags {
-  pub help: bool,
   pub log_debug: bool,
   pub version: bool,
   pub reload: bool,
@@ -28,16 +26,6 @@ pub struct DenoFlags {
   pub fmt: bool,
 }
 
-pub fn get_usage(opts: &Options) -> String {
-  format!(
-    "Usage: deno script.ts {}
-Environment variables:
-        DENO_DIR        Set deno's base directory
-        NO_COLOR        Set to disable color",
-    opts.usage("")
-  )
-}
-
 /// Checks provided arguments for known options and sets appropriate Deno flags
 /// for them. Unknown options are returned for further use.
 /// Note:
@@ -51,129 +39,217 @@ Environment variables:
 /// not cause an error. I also think this is ok because missing any of the
 /// privileged flags is not destructive. Userland flag parsing would catch these
 /// errors.
-fn set_recognized_flags(
-  opts: &Options,
-  flags: &mut DenoFlags,
-  args: Vec<String>,
-) -> Result<Vec<String>, getopts::Fail> {
-  let mut rest = Vec::<String>::new();
-  // getopts doesn't allow parsing unknown options so we check them
-  // one-by-one and handle unrecognized ones manually
-  // better solution welcome!
-  for arg in args {
-    let fake_args = vec![arg];
-    match opts.parse(&fake_args) {
-      Err(getopts::Fail::UnrecognizedOption(_)) => {
-        rest.extend(fake_args);
-      }
-      Err(e) => {
-        return Err(e);
-      }
-      Ok(matches) => {
-        if matches.opt_present("help") {
-          flags.help = true;
-        }
-        if matches.opt_present("log-debug") {
-          flags.log_debug = true;
-        }
-        if matches.opt_present("version") {
-          flags.version = true;
-        }
-        if matches.opt_present("reload") {
-          flags.reload = true;
-        }
-        if matches.opt_present("allow-read") {
-          flags.allow_read = true;
-        }
-        if matches.opt_present("allow-write") {
-          flags.allow_write = true;
-        }
-        if matches.opt_present("allow-net") {
-          flags.allow_net = true;
-        }
-        if matches.opt_present("allow-env") {
-          flags.allow_env = true;
-        }
-        if matches.opt_present("allow-run") {
-          flags.allow_run = true;
-        }
-        if matches.opt_present("allow-all") {
-          flags.allow_read = true;
-          flags.allow_env = true;
-          flags.allow_net = true;
-          flags.allow_run = true;
-          flags.allow_read = true;
-          flags.allow_write = true;
-        }
-        if matches.opt_present("no-prompt") {
-          flags.no_prompts = true;
-        }
-        if matches.opt_present("types") {
-          flags.types = true;
-        }
-        if matches.opt_present("prefetch") {
-          flags.prefetch = true;
-        }
-        if matches.opt_present("info") {
-          flags.info = true;
-        }
-        if matches.opt_present("fmt") {
-          flags.fmt = true;
-        }
-
-        if !matches.free.is_empty() {
-          rest.extend(matches.free);
-        }
-      }
-    }
+fn set_recognized_flags(matches: ArgMatches, flags: &mut DenoFlags) {
+  if matches.is_present("log-debug") {
+    flags.log_debug = true;
+  }
+  if matches.is_present("version") {
+    flags.version = true;
+  }
+  if matches.is_present("reload") {
+    flags.reload = true;
+  }
+  if matches.is_present("allow-read") {
+    flags.allow_read = true;
+  }
+  if matches.is_present("allow-write") {
+    flags.allow_write = true;
+  }
+  if matches.is_present("allow-net") {
+    flags.allow_net = true;
+  }
+  if matches.is_present("allow-env") {
+    flags.allow_env = true;
+  }
+  if matches.is_present("allow-run") {
+    flags.allow_run = true;
+  }
+  if matches.is_present("allow-all") {
+    flags.allow_read = true;
+    flags.allow_env = true;
+    flags.allow_net = true;
+    flags.allow_run = true;
+    flags.allow_read = true;
+    flags.allow_write = true;
+  }
+  if matches.is_present("no-prompt") {
+    flags.no_prompts = true;
+  }
+  if matches.is_present("types") {
+    flags.types = true;
+  }
+  if matches.is_present("prefetch") {
+    flags.prefetch = true;
+  }
+  if matches.is_present("info") {
+    flags.info = true;
+  }
+  if matches.is_present("fmt") {
+    flags.fmt = true;
   }
-  Ok(rest)
 }
 
 #[cfg_attr(feature = "cargo-clippy", allow(stutter))]
 pub fn set_flags(
   args: Vec<String>,
-) -> Result<(DenoFlags, Vec<String>, String), String> {
-  // TODO: all flags passed after "--" are swallowed by v8_set_flags
-  // eg. deno --allow-net ./test.ts -- --title foobar
-  // args === ["deno", "--allow-net" "./test.ts"]
-  let args = v8_set_flags(args);
+) -> Result<(DenoFlags, Vec<String>), String> {
+  let app_settings: Vec<AppSettings> = vec![
+    AppSettings::AllowExternalSubcommands,
+    AppSettings::DisableHelpSubcommand,
+  ];
 
-  let mut opts = Options::new();
-  // TODO(kevinkassimo): v8_set_flags intercepts '-help' with single '-'
-  // Resolve that and then uncomment line below (enabling Go style -long-flag)
-  // opts.long_only(true);
-  opts.optflag("", "allow-read", "Allow file system read access");
-  opts.optflag("", "allow-write", "Allow file system write access");
-  opts.optflag("", "allow-net", "Allow network access");
-  opts.optflag("", "allow-env", "Allow environment access");
-  opts.optflag("", "allow-run", "Allow running subprocesses");
-  opts.optflag("A", "allow-all", "Allow all permissions");
-  opts.optflag("", "no-prompt", "Do not use prompts");
-  opts.optflag("h", "help", "Print this message");
-  opts.optflag("D", "log-debug", "Log debug output");
-  opts.optflag("v", "version", "Print the version");
-  opts.optflag(
-    "r",
-    "reload",
-    "Reload source code cache (recompile TypeScript)",
-  );
-  opts.optflag("", "v8-options", "Print V8 command line options");
-  opts.optflag("", "types", "Print runtime TypeScript declarations");
-  opts.optflag("", "prefetch", "Prefetch the dependencies");
-  opts.optflag("", "info", "Show source file related info");
-  opts.optflag("", "fmt", "Format code");
+  let env_variables_help = "ENVIRONMENT VARIABLES:
+    DENO_DIR        Set deno's base directory
+    NO_COLOR        Set to disable color";
 
-  let mut flags = DenoFlags::default();
+  let clap_app = App::new("deno")
+    .global_settings(&vec![AppSettings::ColorNever])
+    .settings(&app_settings[..])
+    .after_help(env_variables_help)
+    .arg(
+      Arg::with_name("version")
+        .short("v")
+        .long("version")
+        .help("Print the version"),
+    ).arg(
+      Arg::with_name("allow-read")
+        .long("allow-read")
+        .help("Allow file system read access"),
+    ).arg(
+      Arg::with_name("allow-write")
+        .long("allow-write")
+        .help("Allow file system write access"),
+    ).arg(
+      Arg::with_name("allow-net")
+        .long("allow-net")
+        .help("Allow network access"),
+    ).arg(
+      Arg::with_name("allow-env")
+        .long("allow-env")
+        .help("Allow environment access"),
+    ).arg(
+      Arg::with_name("allow-run")
+        .long("allow-run")
+        .help("Allow running subprocesses"),
+    ).arg(
+      Arg::with_name("allow-all")
+        .short("A")
+        .long("allow-all")
+        .help("Allow all permissions"),
+    ).arg(
+      Arg::with_name("no-prompt")
+        .long("no-prompt")
+        .help("Do not use prompts"),
+    ).arg(
+      Arg::with_name("log-debug")
+        .short("D")
+        .long("log-debug")
+        .help("Log debug output"),
+    ).arg(
+      Arg::with_name("reload")
+        .short("r")
+        .long("reload")
+        .help("Reload source code cache (recompile TypeScript)"),
+    ).arg(
+      Arg::with_name("v8-options")
+        .long("v8-options")
+        .help("Print V8 command line options"),
+    ).arg(
+      Arg::with_name("v8-flags")
+        .long("v8-flags")
+        .takes_value(true)
+        .require_equals(true)
+        .help("Set V8 command line options"),
+    ).arg(
+      Arg::with_name("types")
+        .long("types")
+        .help("Print runtime TypeScript declarations"),
+    ).arg(
+      Arg::with_name("prefetch")
+        .long("prefetch")
+        .help("Prefetch the dependencies"),
+    ).subcommand(
+      // TODO(bartlomieju): version is not handled properly
+      SubCommand::with_name("info")
+        .about("Show source file related info")
+        .arg(Arg::with_name("file").takes_value(true).required(true)),
+    ).subcommand(
+      // TODO(bartlomieju): version is not handled properly
+      SubCommand::with_name("fmt").about("Format files").arg(
+        Arg::with_name("files")
+          .takes_value(true)
+          .multiple(true)
+          .required(true),
+      ),
+    ).subcommand(
+      // this is a fake subcommand - it's used in conjunction with
+      // AppSettings:AllowExternalSubcommand to treat it as an
+      // entry point script
+      SubCommand::with_name("<script>").about("Script to run"),
+    );
 
-  let rest =
-    set_recognized_flags(&opts, &mut flags, args).map_err(|e| e.to_string())?;
-  Ok((flags, rest, get_usage(&opts)))
+  let matches = clap_app.get_matches_from(args);
+
+  // TODO(bartomieju): compatibility with old "opts" approach - to be refactored
+  let mut rest: Vec<String> = vec![String::from("deno")];
+
+  match matches.subcommand() {
+    ("info", Some(info_match)) => {
+      // TODO(bartlomieju): it still relies on `is_present("info")` check
+      // in `set_recognized_flags`
+      let file: &str = info_match.value_of("file").unwrap();
+      rest.extend(vec![file.to_string()]);
+    }
+    ("fmt", Some(fmt_match)) => {
+      // TODO(bartlomieju): it still relies on `is_present("fmt")` check
+      // in `set_recognized_flags`
+      let files: Vec<String> = fmt_match
+        .values_of("files")
+        .unwrap()
+        .map(String::from)
+        .collect();
+      rest.extend(files);
+    }
+    (script, Some(script_match)) => {
+      rest.extend(vec![script.to_string()]);
+      // check if there are any extra arguments
+      if script_match.is_present("") {
+        let script_args: Vec<String> = script_match
+          .values_of("")
+          .unwrap()
+          .map(String::from)
+          .collect();
+        rest.extend(script_args);
+      }
+    }
+    _ => {}
+  }
+  // TODO: end
+
+  if matches.is_present("v8-options") {
+    // display v8 help and exit
+    v8_set_flags(vec!["deno".to_string(), "--help".to_string()]);
+  }
+
+  if matches.is_present("v8-flags") {
+    let mut v8_flags: Vec<String> = matches
+      .values_of("v8-flags")
+      .unwrap()
+      .map(String::from)
+      .collect();
+
+    v8_flags.insert(1, "deno".to_string());
+    v8_set_flags(v8_flags);
+  }
+
+  let mut flags = DenoFlags::default();
+  set_recognized_flags(matches, &mut flags);
+  Ok((flags, rest))
 }
 
 #[test]
 fn test_set_flags_1() {
-  let (flags, rest, _) = set_flags(svec!["deno", "--version"]).unwrap();
+  let (flags, rest) = set_flags(svec!["deno", "--version"]).unwrap();
   assert_eq!(rest, svec!["deno"]);
   assert_eq!(
     flags,
@@ -186,7 +262,7 @@ fn test_set_flags_1() {
 
 #[test]
 fn test_set_flags_2() {
-  let (flags, rest, _) =
+  let (flags, rest) =
     set_flags(svec!["deno", "-r", "-D", "script.ts"]).unwrap();
   assert_eq!(rest, svec!["deno", "script.ts"]);
   assert_eq!(
@@ -201,8 +277,8 @@ fn test_set_flags_2() {
 
 #[test]
 fn test_set_flags_3() {
-  let (flags, rest, _) =
-    set_flags(svec!["deno", "-r", "script.ts", "--allow-write"]).unwrap();
+  let (flags, rest) =
+    set_flags(svec!["deno", "-r", "--allow-write", "script.ts"]).unwrap();
   assert_eq!(rest, svec!["deno", "script.ts"]);
   assert_eq!(
     flags,
@@ -216,8 +292,8 @@ fn test_set_flags_3() {
 
 #[test]
 fn test_set_flags_4() {
-  let (flags, rest, _) =
-    set_flags(svec!["deno", "-Dr", "script.ts", "--allow-write"]).unwrap();
+  let (flags, rest) =
+    set_flags(svec!["deno", "-Dr", "--allow-write", "script.ts"]).unwrap();
   assert_eq!(rest, svec!["deno", "script.ts"]);
   assert_eq!(
     flags,
@@ -232,7 +308,7 @@ fn test_set_flags_4() {
 
 #[test]
 fn test_set_flags_5() {
-  let (flags, rest, _) = set_flags(svec!["deno", "--types"]).unwrap();
+  let (flags, rest) = set_flags(svec!["deno", "--types"]).unwrap();
   assert_eq!(rest, svec!["deno"]);
   assert_eq!(
     flags,
@@ -245,8 +321,8 @@ fn test_set_flags_5() {
 
 #[test]
 fn test_set_flags_6() {
-  let (flags, rest, _) =
-    set_flags(svec!["deno", "gist.ts", "--title", "X", "--allow-net"]).unwrap();
+  let (flags, rest) =
+    set_flags(svec!["deno", "--allow-net", "gist.ts", "--title", "X"]).unwrap();
   assert_eq!(rest, svec!["deno", "gist.ts", "--title", "X"]);
   assert_eq!(
     flags,
@@ -259,8 +335,8 @@ fn test_set_flags_6() {
 
 #[test]
 fn test_set_flags_7() {
-  let (flags, rest, _) =
-    set_flags(svec!["deno", "gist.ts", "--allow-all"]).unwrap();
+  let (flags, rest) =
+    set_flags(svec!["deno", "--allow-all", "gist.ts"]).unwrap();
   assert_eq!(rest, svec!["deno", "gist.ts"]);
   assert_eq!(
     flags,
@@ -277,8 +353,8 @@ fn test_set_flags_7() {
 
 #[test]
 fn test_set_flags_8() {
-  let (flags, rest, _) =
-    set_flags(svec!["deno", "gist.ts", "--allow-read"]).unwrap();
+  let (flags, rest) =
+    set_flags(svec!["deno", "--allow-read", "gist.ts"]).unwrap();
   assert_eq!(rest, svec!["deno", "gist.ts"]);
   assert_eq!(
     flags,