Clean up fmt flags and path handling (#3988)

1 files changed, 77 insertions, 72 deletions

diff --git a/cli/fmt.rs b/cli/fmt.rs
index f02df560a..c9b0ef826 100644
--- a/cli/fmt.rs
+++ b/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("**/*")]);
-
-  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);
+pub fn format_files(args: Vec<String>, check: bool) -> Result<(), ErrBox> {
+  if args.len() == 1 && args[0] == "-" {
+    format_stdin(check);
+    return Ok(());
+  }
+
+  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());
+}