Clean up fmt flags and path handling (#3988)

cli/flags.rs

@@ -37,9 +37,9 @@ pub enum DenoSubcommand {
   Fetch {
     files: Vec<String>,
   },
-  Format {
+  Fmt {
     check: bool,
-    files: Option<Vec<PathBuf>>,
+    files: Vec<String>,
   },
   Help,
   Info {
@@ -301,19 +301,13 @@ fn types_parse(flags: &mut DenoFlags, _matches: &clap::ArgMatches) {
 }
 
 fn fmt_parse(flags: &mut DenoFlags, matches: &clap::ArgMatches) {
-  let maybe_files = match matches.values_of("files") {
-    Some(f) => {
-      let files: Vec<PathBuf> = f.map(PathBuf::from).collect();
-      Some(files)
-    }
-    None => None,
+  let files = match matches.values_of("files") {
+    Some(f) => f.map(String::from).collect(),
+    None => vec![],
   };
-
-  let check = matches.is_present("check");
-
-  flags.subcommand = DenoSubcommand::Format {
-    check,
-    files: maybe_files,
+  flags.subcommand = DenoSubcommand::Fmt {
+    check: matches.is_present("check"),
+    files,
   }
 }
 
@@ -533,7 +527,7 @@ The declaration file could be saved and used for typing information.",
 
 fn fmt_subcommand<'a, 'b>() -> App<'a, 'b> {
   SubCommand::with_name("fmt")
-    .about("Format files")
+    .about("Format source files")
     .long_about(
       "Auto-format JavaScript/TypeScript source code
 
@@ -1363,12 +1357,9 @@ mod tests {
     assert_eq!(
       r.unwrap(),
       DenoFlags {
-        subcommand: DenoSubcommand::Format {
+        subcommand: DenoSubcommand::Fmt {
           check: false,
-          files: Some(vec![
-            PathBuf::from("script_1.ts"),
-            PathBuf::from("script_2.ts")
-          ])
+          files: vec!["script_1.ts".to_string(), "script_2.ts".to_string()]
         },
         ..DenoFlags::default()
       }
@@ -1378,9 +1369,9 @@ mod tests {
     assert_eq!(
       r.unwrap(),
       DenoFlags {
-        subcommand: DenoSubcommand::Format {
+        subcommand: DenoSubcommand::Fmt {
           check: true,
-          files: None
+          files: vec![],
         },
         ..DenoFlags::default()
       }
@@ -1390,9 +1381,9 @@ mod tests {
     assert_eq!(
       r.unwrap(),
       DenoFlags {
-        subcommand: DenoSubcommand::Format {
+        subcommand: DenoSubcommand::Fmt {
           check: false,
-          files: None
+          files: vec![],
         },
         ..DenoFlags::default()
       }

cli/fmt.rs

@@ -7,12 +7,9 @@
 //! the future it can be easily extended to provide
 //! the same functions as ops available in JS runtime.
 
-use crate::deno_error::DenoError;
-use crate::deno_error::ErrorKind;
 use deno_core::ErrBox;
 use dprint_plugin_typescript as dprint;
 use glob;
-use regex::Regex;
 use std::fs;
 use std::io::stdin;
 use std::io::stdout;
@@ -22,16 +19,20 @@ use std::path::Path;
 use std::path::PathBuf;
 use std::time::Instant;
 
-lazy_static! {
-  static ref TYPESCRIPT_LIB: Regex = Regex::new(".d.ts$").unwrap();
-  static ref TYPESCRIPT: Regex = Regex::new(".tsx?$").unwrap();
-  static ref JAVASCRIPT: Regex = Regex::new(".jsx?$").unwrap();
-}
-
 fn is_supported(path: &Path) -> bool {
-  let path_str = path.to_string_lossy();
-  !TYPESCRIPT_LIB.is_match(&path_str)
-    && (TYPESCRIPT.is_match(&path_str) || JAVASCRIPT.is_match(&path_str))
+  if let Some(ext) = path.extension() {
+    if ext == "tsx" || ext == "js" || ext == "jsx" {
+      true
+    } else if ext == "ts" {
+      // Currently dprint does not support d.ts files.
+      // https://github.com/dsherret/dprint/issues/100
+      !path.as_os_str().to_string_lossy().ends_with(".d.ts")
+    } else {
+      false
+    }
+  } else {
+    false
+  }
 }
 
 fn get_config() -> dprint::configuration::Configuration {
@@ -47,22 +48,10 @@ fn get_config() -> dprint::configuration::Configuration {
     .build()
 }
 
-fn get_supported_files(paths: Vec<PathBuf>) -> Vec<PathBuf> {
-  let mut files_to_check = vec![];
-
-  for path in paths {
-    if is_supported(&path) {
-      files_to_check.push(path.to_owned());
-    }
-  }
-
-  files_to_check
-}
-
 fn check_source_files(
   config: dprint::configuration::Configuration,
   paths: Vec<PathBuf>,
-) {
+) -> Result<(), ErrBox> {
   let start = Instant::now();
   let mut not_formatted_files = vec![];
 
@@ -75,7 +64,6 @@ fn check_source_files(
       }
       Ok(Some(formatted_text)) => {
         if formatted_text != file_contents {
-          println!("Not formatted {}", file_path_str);
           not_formatted_files.push(file_path);
         }
       }
@@ -88,20 +76,20 @@ fn check_source_files(
 
   let duration = Instant::now() - start;
 
-  if !not_formatted_files.is_empty() {
+  if not_formatted_files.is_empty() {
+    Ok(())
+  } else {
     let f = if not_formatted_files.len() == 1 {
       "file"
     } else {
       "files"
     };
-
-    eprintln!(
+    Err(crate::deno_error::other_error(format!(
       "Found {} not formatted {} in {:?}",
       not_formatted_files.len(),
       f,
       duration
-    );
-    std::process::exit(1);
+    )))
   }
 }
 
@@ -147,56 +135,57 @@ fn format_source_files(
   );
 }
 
-fn get_matching_files(glob_paths: Vec<PathBuf>) -> Vec<PathBuf> {
-  let mut target_files = Vec::with_capacity(128);
-
-  for path in glob_paths {
-    let files = glob::glob(&path.to_str().unwrap())
-      .expect("Failed to execute glob.")
-      .filter_map(Result::ok);
-    target_files.extend(files);
-  }
-
-  target_files
+pub fn source_files_in_subtree(root: PathBuf) -> Vec<PathBuf> {
+  assert!(root.is_dir());
+  // TODO(ry) Use WalkDir instead of globs.
+  let g = root.join("**/*");
+  glob::glob(&g.into_os_string().into_string().unwrap())
+    .expect("Failed to execute glob.")
+    .filter_map(|result| {
+      if let Ok(p) = result {
+        if is_supported(&p) {
+          Some(p)
+        } else {
+          None
+        }
+      } else {
+        None
+      }
+    })
+    .collect()
 }
 
 /// Format JavaScript/TypeScript files.
 ///
 /// First argument supports globs, and if it is `None`
 /// then the current directory is recursively walked.
-pub fn format_files(
-  maybe_files: Option<Vec<PathBuf>>,
-  check: bool,
-) -> Result<(), ErrBox> {
-  // TODO: improve glob to look for tsx?/jsx? files only
-  let glob_paths = maybe_files.unwrap_or_else(|| vec![PathBuf::from("**/*")]);
+pub fn format_files(args: Vec<String>, check: bool) -> Result<(), ErrBox> {
+  if args.len() == 1 && args[0] == "-" {
+    format_stdin(check);
+    return Ok(());
+  }
 
-  for glob_path in glob_paths.iter() {
-    if glob_path.to_str().unwrap() == "-" {
-      // If the only given path is '-', format stdin.
-      if glob_paths.len() == 1 {
-        format_stdin(check);
+  let mut target_files: Vec<PathBuf> = vec![];
+
+  if args.is_empty() {
+    target_files
+      .extend(source_files_in_subtree(std::env::current_dir().unwrap()));
+  } else {
+    for arg in args {
+      let p = PathBuf::from(arg);
+      if p.is_dir() {
+        target_files.extend(source_files_in_subtree(p));
       } else {
-        // Otherwise it is an error
-        return Err(ErrBox::from(DenoError::new(
-          ErrorKind::Other,
-          "Ambiguous filename input. To format stdin, provide a single '-' instead.".to_owned()
-        )));
-      }
-      return Ok(());
+        target_files.push(p);
+      };
     }
   }
-
-  let matching_files = get_matching_files(glob_paths);
-  let matching_files = get_supported_files(matching_files);
   let config = get_config();
-
   if check {
-    check_source_files(config, matching_files);
+    check_source_files(config, target_files)?;
   } else {
-    format_source_files(config, matching_files);
+    format_source_files(config, target_files);
   }
-
   Ok(())
 }
 
@@ -211,10 +200,7 @@ fn format_stdin(check: bool) {
   let config = get_config();
 
   match dprint::format_text("_stdin.ts", &source, &config) {
-    Ok(None) => {
-      // Should not happen.
-      unreachable!();
-    }
+    Ok(None) => unreachable!(),
     Ok(Some(formatted_text)) => {
       if check {
         if formatted_text != source {
@@ -231,3 +217,22 @@ fn format_stdin(check: bool) {
     }
   }
 }
+
+#[test]
+fn test_is_supported() {
+  assert!(!is_supported(Path::new("tests/subdir/redirects")));
+  assert!(!is_supported(Path::new("README.md")));
+  assert!(!is_supported(Path::new("lib/typescript.d.ts")));
+  assert!(is_supported(Path::new("cli/tests/001_hello.js")));
+  assert!(is_supported(Path::new("cli/tests/002_hello.ts")));
+  assert!(is_supported(Path::new("foo.jsx")));
+  assert!(is_supported(Path::new("foo.tsx")));
+}
+
+#[test]
+fn check_tests_dir() {
+  // Because of cli/tests/error_syntax.js the following should fail but not
+  // crash.
+  let r = format_files(vec!["./tests".to_string()], true);
+  assert!(r.is_err());
+}

cli/lib.rs

@@ -413,7 +413,7 @@ async fn run_script(
   Ok(())
 }
 
-async fn fmt_command(files: Option<Vec<PathBuf>>, check: bool) {
+async fn fmt_command(files: Vec<String>, check: bool) {
   if let Err(err) = fmt::format_files(files, check) {
     print_err_and_exit(err);
   }
@@ -493,9 +493,7 @@ pub fn main() {
       }
       DenoSubcommand::Eval { code } => eval_command(flags, code).await,
       DenoSubcommand::Fetch { files } => fetch_command(flags, files).await,
-      DenoSubcommand::Format { check, files } => {
-        fmt_command(files, check).await
-      }
+      DenoSubcommand::Fmt { check, files } => fmt_command(files, check).await,
       DenoSubcommand::Info { file } => info_command(flags, file).await,
       DenoSubcommand::Install {
         dir,

cli/tests/integration_tests.rs

@@ -641,14 +641,6 @@ itest!(fmt_stdin {
   output_str: Some("const a = 1;\n"),
 });
 
-itest!(fmt_stdin_ambiguous {
-  args: "fmt - file.ts",
-  input: Some("const a = 1\n"),
-  check_stderr: true,
-  exit_code: 1,
-  output_str: Some("Ambiguous filename input. To format stdin, provide a single '-' instead.\n"),
-});
-
 itest!(fmt_stdin_check_formatted {
   args: "fmt --check -",
   input: Some("const a = 1;\n"),