From a913b7a1ba67849464d0314eee9851f59c22556b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Mon, 4 May 2020 13:03:30 +0200 Subject: [PATCH] BREAKING: remove CLI 'deno script.ts' hack (#5026) This PR removes the hack in CLI that allows to run scripts with shorthand: deno script.ts. Removing this functionality because it hacks around short-comings of clap our CLI parser. We agree that this shorthand syntax is desirable, but it needs to be rethinked and reimplemented. For 1.0 we should go with conservative approach that is correct. --- cli/flags.rs | 158 +++---------------------- cli/tests/integration_tests.rs | 34 +++--- cli/tests/lock_write_fetch.ts | 7 +- std/examples/chat/server_test.ts | 1 + std/examples/tests/cat_test.ts | 1 + std/examples/tests/catj_test.ts | 2 +- std/examples/tests/colors_test.ts | 2 +- std/examples/tests/curl_test.ts | 8 +- std/examples/tests/echo_server_test.ts | 2 +- std/examples/tests/welcome_test.ts | 2 +- std/examples/tests/xeval_test.ts | 4 +- std/fs/expand_glob_test.ts | 2 +- test_plugin/tests/integration_tests.rs | 1 + tools/benchmark.py | 19 +-- 14 files changed, 66 insertions(+), 177 deletions(-) diff --git a/cli/flags.rs b/cli/flags.rs index 866b368206..239382e03d 100644 --- a/cli/flags.rs +++ b/cli/flags.rs @@ -6,7 +6,6 @@ use clap::Arg; use clap::ArgMatches; use clap::SubCommand; use log::Level; -use std::collections::HashSet; use std::net::SocketAddr; use std::path::{Path, PathBuf}; @@ -14,14 +13,6 @@ use std::path::{Path, PathBuf}; macro_rules! svec { ($($x:expr),*) => (vec![$($x.to_string()),*]); } -/// Creates HashSet from string literals -macro_rules! sset { - ($($x:expr),*) => {{ - let _v = svec![$($x.to_string()),*]; - let hash_set: HashSet = _v.iter().cloned().collect(); - hash_set - }} -} #[derive(Clone, Debug, PartialEq)] pub enum DenoSubcommand { @@ -196,17 +187,15 @@ Docs: https://deno.land/std/manual.md Modules: https://deno.land/std/ https://deno.land/x/ Bugs: https://github.com/denoland/deno/issues -To start the REPL, supply no arguments: +To start the REPL: deno To execute a script: deno run https://deno.land/std/examples/welcome.ts - deno https://deno.land/std/examples/welcome.ts To evaluate code in the shell: deno eval \"console.log(30933 + 404)\" - -Run 'deno help run' for 'run'-specific flags."; +"; lazy_static! { static ref LONG_VERSION: String = format!( @@ -228,7 +217,6 @@ pub fn flags_from_vec(args: Vec) -> Flags { /// Same as flags_from_vec but does not exit on error. pub fn flags_from_vec_safe(args: Vec) -> clap::Result { - let args = arg_hacks(args); let app = clap_root(); let matches = app.get_matches_from_safe(args)?; @@ -272,7 +260,7 @@ pub fn flags_from_vec_safe(args: Vec) -> clap::Result { } else if let Some(m) = matches.subcommand_matches("doc") { doc_parse(&mut flags, m); } else { - unimplemented!(); + repl_parse(&mut flags, &matches); } Ok(flags) @@ -1342,79 +1330,11 @@ fn resolve_hosts(paths: Vec) -> Vec { out } -fn arg_hacks(mut args: Vec) -> Vec { - // Hack #1 We want to default the subcommand to "run" - // Clap does not let us have a default sub-command. But we want to allow users - // to do "deno script.js" instead of "deno run script.js". - // This function insert the "run" into the second position of the args. - assert!(!args.is_empty()); - // Rational: - // deno -> deno repl - if args.len() == 1 { - args.insert(1, "repl".to_string()); - return args; - } - let subcommands = sset![ - "bundle", - "completions", - "doc", - "eval", - "cache", - "fmt", - "test", - "info", - "repl", - "run", - "types", - "install", - "help", - "version", - "upgrade" - ]; - let modifier_flags = sset!["-h", "--help", "-V", "--version"]; - // deno [subcommand|behavior modifier flags] -> do nothing - if subcommands.contains(&args[1]) || modifier_flags.contains(&args[1]) { - return args; - } - // This is not perfect either, since originally we should also - // support e.g. `-L debug` which `debug` would be treated as main module. - // Instead `-L=debug` must be used - let mut has_main_module = false; - for arg in args.iter().skip(1) { - if !arg.starts_with('-') { - has_main_module = true; - break; - } - } - if has_main_module { - // deno ...-[flags] NAME ... -> deno run ...-[flags] NAME ... - args.insert(1, "run".to_string()); - } else { - // deno ...-[flags] -> deno repl ...-[flags] - args.insert(1, "repl".to_string()); - } - args -} - #[cfg(test)] mod tests { use super::*; use std::env::current_dir; - #[test] - fn arg_hacks_test() { - let args0 = arg_hacks(svec!["deno", "--version"]); - assert_eq!(args0, ["deno", "--version"]); - let args1 = arg_hacks(svec!["deno"]); - assert_eq!(args1, ["deno", "repl"]); - let args2 = arg_hacks(svec!["deno", "-L=debug", "-h"]); - assert_eq!(args2, ["deno", "repl", "-L=debug", "-h"]); - let args3 = arg_hacks(svec!["deno", "script.js"]); - assert_eq!(args3, ["deno", "run", "script.js"]); - let args4 = arg_hacks(svec!["deno", "-A", "script.js", "-L=info"]); - assert_eq!(args4, ["deno", "run", "-A", "script.js", "-L=info"]); - } - #[test] fn upgrade() { let r = @@ -1967,41 +1887,6 @@ mod tests { ); } - #[test] - fn default_to_run() { - let r = flags_from_vec_safe(svec!["deno", "script.ts"]); - assert_eq!( - r.unwrap(), - Flags { - subcommand: DenoSubcommand::Run { - script: "script.ts".to_string(), - }, - ..Flags::default() - } - ); - } - - #[test] - fn default_to_run_with_permissions() { - let r = flags_from_vec_safe(svec![ - "deno", - "--allow-net", - "--allow-read", - "script.ts" - ]); - assert_eq!( - r.unwrap(), - Flags { - subcommand: DenoSubcommand::Run { - script: "script.ts".to_string(), - }, - allow_net: true, - allow_read: true, - ..Flags::default() - } - ); - } - #[test] fn bundle() { let r = flags_from_vec_safe(svec!["deno", "bundle", "source.ts"]); @@ -2071,25 +1956,6 @@ mod tests { ); } - #[test] - fn default_to_run_importmap() { - let r = flags_from_vec_safe(svec![ - "deno", - "--importmap=importmap.json", - "script.ts" - ]); - assert_eq!( - r.unwrap(), - Flags { - subcommand: DenoSubcommand::Run { - script: "script.ts".to_string(), - }, - import_map_path: Some("importmap.json".to_owned()), - ..Flags::default() - } - ); - } - #[test] fn cache_importmap() { let r = flags_from_vec_safe(svec![ @@ -2250,8 +2116,12 @@ mod tests { #[test] fn log_level() { - let r = - flags_from_vec_safe(svec!["deno", "--log-level=debug", "script.ts"]); + let r = flags_from_vec_safe(svec![ + "deno", + "run", + "--log-level=debug", + "script.ts" + ]); assert_eq!( r.unwrap(), Flags { @@ -2266,7 +2136,7 @@ mod tests { #[test] fn quiet() { - let r = flags_from_vec_safe(svec!["deno", "-q", "script.ts"]); + let r = flags_from_vec_safe(svec!["deno", "run", "-q", "script.ts"]); assert_eq!( r.unwrap(), Flags { @@ -2350,7 +2220,8 @@ mod tests { #[test] fn no_remote() { - let r = flags_from_vec_safe(svec!["deno", "--no-remote", "script.ts"]); + let r = + flags_from_vec_safe(svec!["deno", "run", "--no-remote", "script.ts"]); assert_eq!( r.unwrap(), Flags { @@ -2365,7 +2236,8 @@ mod tests { #[test] fn cached_only() { - let r = flags_from_vec_safe(svec!["deno", "--cached-only", "script.ts"]); + let r = + flags_from_vec_safe(svec!["deno", "run", "--cached-only", "script.ts"]); assert_eq!( r.unwrap(), Flags { @@ -2382,6 +2254,7 @@ mod tests { fn allow_net_whitelist_with_ports() { let r = flags_from_vec_safe(svec![ "deno", + "run", "--allow-net=deno.land,:8000,:4545", "script.ts" ]); @@ -2409,6 +2282,7 @@ mod tests { fn lock_write() { let r = flags_from_vec_safe(svec![ "deno", + "run", "--lock-write", "--lock=lock.json", "script.ts" diff --git a/cli/tests/integration_tests.rs b/cli/tests/integration_tests.rs index 7827d386a1..1d02226b49 100644 --- a/cli/tests/integration_tests.rs +++ b/cli/tests/integration_tests.rs @@ -1322,7 +1322,7 @@ itest!(error_014_catch_dynamic_import_error { }); itest!(error_015_dynamic_import_permissions { - args: "--reload error_015_dynamic_import_permissions.js", + args: "run --reload error_015_dynamic_import_permissions.js", output: "error_015_dynamic_import_permissions.out", check_stderr: true, exit_code: 1, @@ -1331,7 +1331,7 @@ itest!(error_015_dynamic_import_permissions { // We have an allow-net flag but not allow-read, it should still result in error. itest!(error_016_dynamic_import_permissions2 { - args: "--reload --allow-net error_016_dynamic_import_permissions2.js", + args: "run --reload --allow-net error_016_dynamic_import_permissions2.js", output: "error_016_dynamic_import_permissions2.out", check_stderr: true, exit_code: 1, @@ -1339,63 +1339,63 @@ itest!(error_016_dynamic_import_permissions2 { }); itest!(error_017_hide_long_source_ts { - args: "--reload error_017_hide_long_source_ts.ts", + args: "run --reload error_017_hide_long_source_ts.ts", output: "error_017_hide_long_source_ts.ts.out", check_stderr: true, exit_code: 1, }); itest!(error_018_hide_long_source_js { - args: "error_018_hide_long_source_js.js", + args: "run error_018_hide_long_source_js.js", output: "error_018_hide_long_source_js.js.out", check_stderr: true, exit_code: 1, }); itest!(error_019_stack_function { - args: "error_019_stack_function.ts", + args: "run error_019_stack_function.ts", output: "error_019_stack_function.ts.out", check_stderr: true, exit_code: 1, }); itest!(error_020_stack_constructor { - args: "error_020_stack_constructor.ts", + args: "run error_020_stack_constructor.ts", output: "error_020_stack_constructor.ts.out", check_stderr: true, exit_code: 1, }); itest!(error_021_stack_method { - args: "error_021_stack_method.ts", + args: "run error_021_stack_method.ts", output: "error_021_stack_method.ts.out", check_stderr: true, exit_code: 1, }); itest!(error_022_stack_custom_error { - args: "error_022_stack_custom_error.ts", + args: "run error_022_stack_custom_error.ts", output: "error_022_stack_custom_error.ts.out", check_stderr: true, exit_code: 1, }); itest!(error_023_stack_async { - args: "error_023_stack_async.ts", + args: "run error_023_stack_async.ts", output: "error_023_stack_async.ts.out", check_stderr: true, exit_code: 1, }); itest!(error_024_stack_promise_all { - args: "error_024_stack_promise_all.ts", + args: "run error_024_stack_promise_all.ts", output: "error_024_stack_promise_all.ts.out", check_stderr: true, exit_code: 1, }); itest!(error_025_tab_indent { - args: "error_025_tab_indent", + args: "run error_025_tab_indent", output: "error_025_tab_indent.out", check_stderr: true, exit_code: 1, @@ -1534,7 +1534,7 @@ itest!(run_v8_flags { }); itest!(run_v8_help { - args: "--v8-flags=--help", + args: "repl --v8-flags=--help", output: "v8_help.out", }); @@ -1544,27 +1544,27 @@ itest!(wasm { }); itest!(wasm_async { - args: "wasm_async.js", + args: "run wasm_async.js", output: "wasm_async.out", }); itest!(top_level_await { - args: "--allow-read top_level_await.js", + args: "run --allow-read top_level_await.js", output: "top_level_await.out", }); itest!(top_level_await_ts { - args: "--allow-read top_level_await.ts", + args: "run --allow-read top_level_await.ts", output: "top_level_await.out", }); itest!(top_level_for_await { - args: "top_level_for_await.js", + args: "run top_level_for_await.js", output: "top_level_for_await.out", }); itest!(top_level_for_await_ts { - args: "top_level_for_await.ts", + args: "run top_level_for_await.ts", output: "top_level_for_await.out", }); diff --git a/cli/tests/lock_write_fetch.ts b/cli/tests/lock_write_fetch.ts index 2286d7a0c4..9e71905965 100644 --- a/cli/tests/lock_write_fetch.ts +++ b/cli/tests/lock_write_fetch.ts @@ -35,7 +35,12 @@ console.log(`fetch check code: ${fetchCheckProcCode}`); const runProc = Deno.run({ stdout: "null", stderr: "null", - cmd: [Deno.execPath(), "--lock=lock_write_fetch.json", "https_import.ts"], + cmd: [ + Deno.execPath(), + "run", + "--lock=lock_write_fetch.json", + "https_import.ts", + ], }); const runCode = (await runProc.status()).code; diff --git a/std/examples/chat/server_test.ts b/std/examples/chat/server_test.ts index 76b1f1bf54..d1c1a8afa9 100644 --- a/std/examples/chat/server_test.ts +++ b/std/examples/chat/server_test.ts @@ -12,6 +12,7 @@ async function startServer(): Promise { // TODO(lucacasonato): remove unstable once possible cmd: [ Deno.execPath(), + "run", "--allow-net", "--allow-read", "--unstable", diff --git a/std/examples/tests/cat_test.ts b/std/examples/tests/cat_test.ts index b12a6916f6..4633c5750c 100644 --- a/std/examples/tests/cat_test.ts +++ b/std/examples/tests/cat_test.ts @@ -6,6 +6,7 @@ Deno.test("[examples/cat] print multiple files", async () => { const process = Deno.run({ cmd: [ Deno.execPath(), + "run", "--allow-read", "cat.ts", "testdata/cat/hello.txt", diff --git a/std/examples/tests/catj_test.ts b/std/examples/tests/catj_test.ts index 53b36bef83..c3eb61f515 100644 --- a/std/examples/tests/catj_test.ts +++ b/std/examples/tests/catj_test.ts @@ -77,7 +77,7 @@ Deno.test("[examples/catj] read from stdin", async () => { function catj(...files: string[]): Deno.Process { return Deno.run({ - cmd: [Deno.execPath(), "--allow-read", "catj.ts", ...files], + cmd: [Deno.execPath(), "run", "--allow-read", "catj.ts", ...files], cwd: "examples", stdin: "piped", stdout: "piped", diff --git a/std/examples/tests/colors_test.ts b/std/examples/tests/colors_test.ts index ac779adb8d..90c469c009 100644 --- a/std/examples/tests/colors_test.ts +++ b/std/examples/tests/colors_test.ts @@ -4,7 +4,7 @@ import { assertStrictEq } from "../../testing/asserts.ts"; Deno.test("[examples/colors] print a colored text", async () => { const decoder = new TextDecoder(); const process = Deno.run({ - cmd: [Deno.execPath(), "colors.ts"], + cmd: [Deno.execPath(), "run", "colors.ts"], cwd: "examples", stdout: "piped", }); diff --git a/std/examples/tests/curl_test.ts b/std/examples/tests/curl_test.ts index 9b2870216f..69e0e52d6b 100644 --- a/std/examples/tests/curl_test.ts +++ b/std/examples/tests/curl_test.ts @@ -14,7 +14,13 @@ Deno.test({ const decoder = new TextDecoder(); const process = Deno.run({ - cmd: [Deno.execPath(), "--allow-net", "curl.ts", "http://localhost:8081"], + cmd: [ + Deno.execPath(), + "run", + "--allow-net", + "curl.ts", + "http://localhost:8081", + ], cwd: "examples", stdout: "piped", }); diff --git a/std/examples/tests/echo_server_test.ts b/std/examples/tests/echo_server_test.ts index 9c413d737d..3e60961909 100644 --- a/std/examples/tests/echo_server_test.ts +++ b/std/examples/tests/echo_server_test.ts @@ -6,7 +6,7 @@ Deno.test("[examples/echo_server]", async () => { const encoder = new TextEncoder(); const decoder = new TextDecoder(); const process = Deno.run({ - cmd: [Deno.execPath(), "--allow-net", "echo_server.ts"], + cmd: [Deno.execPath(), "run", "--allow-net", "echo_server.ts"], cwd: "examples", stdout: "piped", }); diff --git a/std/examples/tests/welcome_test.ts b/std/examples/tests/welcome_test.ts index cacb77ab9d..d508773c55 100644 --- a/std/examples/tests/welcome_test.ts +++ b/std/examples/tests/welcome_test.ts @@ -4,7 +4,7 @@ import { assertStrictEq } from "../../testing/asserts.ts"; Deno.test("[examples/welcome] print a welcome message", async () => { const decoder = new TextDecoder(); const process = Deno.run({ - cmd: [Deno.execPath(), "welcome.ts"], + cmd: [Deno.execPath(), "run", "welcome.ts"], cwd: "examples", stdout: "piped", }); diff --git a/std/examples/tests/xeval_test.ts b/std/examples/tests/xeval_test.ts index 426f6fe568..eeeac7731a 100644 --- a/std/examples/tests/xeval_test.ts +++ b/std/examples/tests/xeval_test.ts @@ -29,7 +29,7 @@ Deno.test({ name: "xevalCliReplvar", fn: async function (): Promise { const p = run({ - cmd: [execPath(), xevalPath, "--replvar=abc", "console.log(abc)"], + cmd: [execPath(), "run", xevalPath, "--replvar=abc", "console.log(abc)"], stdin: "piped", stdout: "piped", stderr: "null", @@ -45,7 +45,7 @@ Deno.test({ Deno.test("xevalCliSyntaxError", async function (): Promise { const p = run({ - cmd: [execPath(), xevalPath, "("], + cmd: [execPath(), "run", xevalPath, "("], stdin: "null", stdout: "piped", stderr: "piped", diff --git a/std/fs/expand_glob_test.ts b/std/fs/expand_glob_test.ts index 697c8bf98e..24885530bb 100644 --- a/std/fs/expand_glob_test.ts +++ b/std/fs/expand_glob_test.ts @@ -115,7 +115,7 @@ Deno.test("expandGlobIncludeDirs", async function (): Promise { Deno.test("expandGlobPermError", async function (): Promise { const exampleUrl = new URL("testdata/expand_wildcard.js", import.meta.url); const p = run({ - cmd: [execPath(), "--unstable", exampleUrl.toString()], + cmd: [execPath(), "run", "--unstable", exampleUrl.toString()], stdin: "null", stdout: "piped", stderr: "piped", diff --git a/test_plugin/tests/integration_tests.rs b/test_plugin/tests/integration_tests.rs index 1f118b77a9..2f61ec9aa5 100644 --- a/test_plugin/tests/integration_tests.rs +++ b/test_plugin/tests/integration_tests.rs @@ -30,6 +30,7 @@ fn basic() { let build_plugin_output = build_plugin.output().unwrap(); assert!(build_plugin_output.status.success()); let output = deno_cmd() + .arg("run") .arg("--allow-plugin") .arg("--unstable") .arg("tests/test.js") diff --git a/tools/benchmark.py b/tools/benchmark.py index a921feaf89..9485d79f4a 100755 --- a/tools/benchmark.py +++ b/tools/benchmark.py @@ -20,15 +20,16 @@ import http_server # The list of the tuples of the benchmark name and arguments exec_time_benchmarks = [ - ("hello", ["cli/tests/002_hello.ts"]), - ("relative_import", ["cli/tests/003_relative_import.ts"]), - ("error_001", ["cli/tests/error_001.ts"]), - ("cold_hello", ["--reload", "cli/tests/002_hello.ts"]), - ("cold_relative_import", ["--reload", "cli/tests/003_relative_import.ts"]), - ("workers_startup", ["cli/tests/workers_startup_bench.ts"]), - ("workers_round_robin", ["cli/tests/workers_round_robin_bench.ts"]), - ("text_decoder", ["cli/tests/text_decoder_perf.js"]), - ("text_encoder", ["cli/tests/text_encoder_perf.js"]), + ("hello", ["run", "cli/tests/002_hello.ts"]), + ("relative_import", ["run", "cli/tests/003_relative_import.ts"]), + ("error_001", ["run", "cli/tests/error_001.ts"]), + ("cold_hello", ["run", "--reload", "cli/tests/002_hello.ts"]), + ("cold_relative_import", + ["run", "--reload", "cli/tests/003_relative_import.ts"]), + ("workers_startup", ["run", "cli/tests/workers_startup_bench.ts"]), + ("workers_round_robin", ["run", "cli/tests/workers_round_robin_bench.ts"]), + ("text_decoder", ["run", "cli/tests/text_decoder_perf.js"]), + ("text_encoder", ["run", "cli/tests/text_encoder_perf.js"]), ]