From 2d9d69f25bcd063a54fff149809b1aa7270c3fb9 Mon Sep 17 00:00:00 2001 From: Ryan Dahl Date: Thu, 13 Feb 2020 11:55:46 -0500 Subject: [PATCH 1/7] simplify fmt path handling, add warning --- cli/flags.rs | 41 +++++++++++++--------------------- cli/fmt.rs | 63 ++++++++++++++++++---------------------------------- cli/lib.rs | 6 ++--- 3 files changed, 40 insertions(+), 70 deletions(-) diff --git a/cli/flags.rs b/cli/flags.rs index 5326dc2874f2a7..7ffdd7b919d282 100644 --- a/cli/flags.rs +++ b/cli/flags.rs @@ -37,9 +37,9 @@ pub enum DenoSubcommand { Fetch { files: Vec, }, - Format { + Fmt { check: bool, - files: Option>, + files: Vec, }, 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 = 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("Fmt files") .long_about( "Auto-format JavaScript/TypeScript source code @@ -543,7 +537,7 @@ fn fmt_subcommand<'a, 'b>() -> App<'a, 'b> { deno fmt --check - # Format stdin and write to stdout + # Fmt stdin and write to stdout cat file.ts | deno fmt -", ) .arg( @@ -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() } diff --git a/cli/fmt.rs b/cli/fmt.rs index f02df560a4c3fe..6c9a4f0ae2087e 100644 --- a/cli/fmt.rs +++ b/cli/fmt.rs @@ -7,8 +7,6 @@ //! 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; @@ -147,48 +145,34 @@ fn format_source_files( ); } -fn get_matching_files(glob_paths: Vec) -> Vec { - 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 -} - /// 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>, - 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); - } 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(()); +pub fn format_files(mut files: Vec, check: bool) -> Result<(), ErrBox> { + if files.len() == 1 && files[0] == "-" { + format_stdin(check); + return Ok(()); + } + + if files.is_empty() { + files.push("**/*".to_string()); + } + + let mut target_files: Vec = Vec::new(); + + for f in files { + let files: Vec = glob::glob(&f) + .expect("Failed to execute glob.") + .filter_map(Result::ok) + .collect(); + if files.is_empty() { + eprintln!("Warning: '{}' does not match any files", f); } + target_files.extend(files); } - let matching_files = get_matching_files(glob_paths); - let matching_files = get_supported_files(matching_files); + let matching_files = get_supported_files(target_files); let config = get_config(); if check { @@ -211,10 +195,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 { diff --git a/cli/lib.rs b/cli/lib.rs index f89eb4bd74c602..e646c4199925fe 100644 --- a/cli/lib.rs +++ b/cli/lib.rs @@ -413,7 +413,7 @@ async fn run_script( Ok(()) } -async fn fmt_command(files: Option>, check: bool) { +async fn fmt_command(files: Vec, 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, From b4359b163ea7399ae2d1b2ba306a37b48a92e509 Mon Sep 17 00:00:00 2001 From: Ryan Dahl Date: Thu, 13 Feb 2020 12:14:16 -0500 Subject: [PATCH 2/7] cleanup --- cli/fmt.rs | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/cli/fmt.rs b/cli/fmt.rs index 6c9a4f0ae2087e..ce364422f986ee 100644 --- a/cli/fmt.rs +++ b/cli/fmt.rs @@ -161,15 +161,31 @@ pub fn format_files(mut files: Vec, check: bool) -> Result<(), ErrBox> { let mut target_files: Vec = Vec::new(); + if files.is_empty() { + target_files.extend( + glob::glob("**/*") + .expect("Failed to execute glob.") + .filter_map(Result::ok) + .collect::>(), + ); + } + for f in files { - let files: Vec = glob::glob(&f) - .expect("Failed to execute glob.") - .filter_map(Result::ok) - .collect(); - if files.is_empty() { - eprintln!("Warning: '{}' does not match any files", f); - } - target_files.extend(files); + let p = PathBuf::from(f.clone()); + if !p.is_dir() { + target_files.push(p); + } else { + let g = p.join("**").join("*"); + let files_: Vec = + glob::glob(&g.into_os_string().into_string().unwrap()) + .expect("Failed to execute glob.") + .filter_map(Result::ok) + .collect(); + if files_.is_empty() { + eprintln!("Warning: '{}' does not match any files", f); + } + target_files.extend(files_); + }; } let matching_files = get_supported_files(target_files); From 070a731dd74680dc4a43990144539d2ff64d0392 Mon Sep 17 00:00:00 2001 From: Ryan Dahl Date: Thu, 13 Feb 2020 12:26:02 -0500 Subject: [PATCH 3/7] cleanup --- cli/fmt.rs | 52 ++++++++++++++++++++++------------------------------ 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/cli/fmt.rs b/cli/fmt.rs index ce364422f986ee..d86632992debc4 100644 --- a/cli/fmt.rs +++ b/cli/fmt.rs @@ -145,47 +145,39 @@ fn format_source_files( ); } +pub fn source_files_in_subtree(root: PathBuf) -> Vec { + assert!(root.is_dir()); + let g = root.join("**/*"); + glob::glob(&g.into_os_string().into_string().unwrap()) + .expect("Failed to execute glob.") + .filter_map(Result::ok) + .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(mut files: Vec, check: bool) -> Result<(), ErrBox> { +pub fn format_files(files: Vec, check: bool) -> Result<(), ErrBox> { if files.len() == 1 && files[0] == "-" { format_stdin(check); return Ok(()); } - if files.is_empty() { - files.push("**/*".to_string()); - } - - let mut target_files: Vec = Vec::new(); + let mut target_files: Vec = vec![]; if files.is_empty() { - target_files.extend( - glob::glob("**/*") - .expect("Failed to execute glob.") - .filter_map(Result::ok) - .collect::>(), - ); - } - - for f in files { - let p = PathBuf::from(f.clone()); - if !p.is_dir() { - target_files.push(p); - } else { - let g = p.join("**").join("*"); - let files_: Vec = - glob::glob(&g.into_os_string().into_string().unwrap()) - .expect("Failed to execute glob.") - .filter_map(Result::ok) - .collect(); - if files_.is_empty() { - eprintln!("Warning: '{}' does not match any files", f); - } - target_files.extend(files_); - }; + target_files + .extend(source_files_in_subtree(std::env::current_dir().unwrap())); + } else { + for f in files { + let p = PathBuf::from(f); + if !p.is_dir() { + target_files.push(p); + } else { + target_files.extend(source_files_in_subtree(p)); + }; + } } let matching_files = get_supported_files(target_files); From deff1b7eecb5c0b33924b8e0e75386dc8e0ab84b Mon Sep 17 00:00:00 2001 From: Ryan Dahl Date: Thu, 13 Feb 2020 13:26:44 -0500 Subject: [PATCH 4/7] cleanup --- cli/fmt.rs | 97 +++++++++++++++++++++++++++++------------------------- 1 file changed, 53 insertions(+), 44 deletions(-) diff --git a/cli/fmt.rs b/cli/fmt.rs index d86632992debc4..1d56723d0a84b6 100644 --- a/cli/fmt.rs +++ b/cli/fmt.rs @@ -10,7 +10,6 @@ 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; @@ -20,16 +19,12 @@ 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() { + ext == "ts" || ext == "tsx" || ext == "js" || ext == "jsx" + } else { + false + } } fn get_config() -> dprint::configuration::Configuration { @@ -45,22 +40,10 @@ fn get_config() -> dprint::configuration::Configuration { .build() } -fn get_supported_files(paths: Vec) -> Vec { - 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, -) { +) -> Result<(), ErrBox> { let start = Instant::now(); let mut not_formatted_files = vec![]; @@ -73,7 +56,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); } } @@ -86,20 +68,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,10 +129,21 @@ fn format_source_files( pub fn source_files_in_subtree(root: PathBuf) -> Vec { 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::ok) + .filter_map(|result| { + if let Ok(p) = result { + if is_supported(&p) { + Some(p) + } else { + None + } + } else { + None + } + }) .collect() } @@ -158,37 +151,33 @@ pub fn source_files_in_subtree(root: PathBuf) -> Vec { /// /// First argument supports globs, and if it is `None` /// then the current directory is recursively walked. -pub fn format_files(files: Vec, check: bool) -> Result<(), ErrBox> { - if files.len() == 1 && files[0] == "-" { +pub fn format_files(args: Vec, check: bool) -> Result<(), ErrBox> { + if args.len() == 1 && args[0] == "-" { format_stdin(check); return Ok(()); } let mut target_files: Vec = vec![]; - if files.is_empty() { + if args.is_empty() { target_files .extend(source_files_in_subtree(std::env::current_dir().unwrap())); } else { - for f in files { - let p = PathBuf::from(f); - if !p.is_dir() { - target_files.push(p); - } else { + for arg in args { + let p = PathBuf::from(arg); + if p.is_dir() { target_files.extend(source_files_in_subtree(p)); + } else { + target_files.push(p); }; } } - - let matching_files = get_supported_files(target_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(()) } @@ -220,3 +209,23 @@ fn format_stdin(check: bool) { } } } + +#[test] +fn test_is_supported() { + assert!(!is_supported(Path::new("tests/subdir/redirects"))); + 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("cli/tests/001_hello.jsx"))); + assert!(is_supported(Path::new("cli/tests/002_hello.tsx"))); + assert!(is_supported(Path::new( + "deno_typescript/typescript/lib/typescript.d.ts" + ))); +} + +#[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()); +} From a0532b6edcc3aa55bf1ee6d234ea5e119ff6d2c6 Mon Sep 17 00:00:00 2001 From: Ryan Dahl Date: Thu, 13 Feb 2020 13:28:33 -0500 Subject: [PATCH 5/7] x --- cli/tests/integration_tests.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/cli/tests/integration_tests.rs b/cli/tests/integration_tests.rs index a41fdd032832cc..35127c155a7e39 100644 --- a/cli/tests/integration_tests.rs +++ b/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"), From 0913310daf20d4609247dec549081a84c0eef34d Mon Sep 17 00:00:00 2001 From: Ryan Dahl Date: Thu, 13 Feb 2020 13:30:39 -0500 Subject: [PATCH 6/7] fix --- cli/flags.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/flags.rs b/cli/flags.rs index 7ffdd7b919d282..445a08c0b7b899 100644 --- a/cli/flags.rs +++ b/cli/flags.rs @@ -527,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("Fmt files") + .about("Format source files") .long_about( "Auto-format JavaScript/TypeScript source code @@ -537,7 +537,7 @@ fn fmt_subcommand<'a, 'b>() -> App<'a, 'b> { deno fmt --check - # Fmt stdin and write to stdout + # Format stdin and write to stdout cat file.ts | deno fmt -", ) .arg( From 85a132e2f202704f619d81d7ac1b79dd8ce8ff81 Mon Sep 17 00:00:00 2001 From: Ryan Dahl Date: Thu, 13 Feb 2020 15:01:35 -0500 Subject: [PATCH 7/7] fix --- cli/fmt.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/cli/fmt.rs b/cli/fmt.rs index 1d56723d0a84b6..c9b0ef82660b5b 100644 --- a/cli/fmt.rs +++ b/cli/fmt.rs @@ -21,7 +21,15 @@ use std::time::Instant; fn is_supported(path: &Path) -> bool { if let Some(ext) = path.extension() { - ext == "ts" || ext == "tsx" || ext == "js" || ext == "jsx" + 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 } @@ -213,13 +221,12 @@ 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("cli/tests/001_hello.jsx"))); - assert!(is_supported(Path::new("cli/tests/002_hello.tsx"))); - assert!(is_supported(Path::new( - "deno_typescript/typescript/lib/typescript.d.ts" - ))); + assert!(is_supported(Path::new("foo.jsx"))); + assert!(is_supported(Path::new("foo.tsx"))); } #[test]