From 776a7c086969b2d00889a2f6585a504f696d729a Mon Sep 17 00:00:00 2001 From: Luis Lavena Date: Mon, 20 May 2024 12:44:20 +0200 Subject: [PATCH 1/4] Pass all ARGV arguments verbatim to subcommand Avoid altering original `ARGV` when combining it with possible `SHARDS_OPTS` variable and pass them verbatim to the subcommand. Introduce a naive integration test for subcommand to validate the change works correctly. --- spec/integration/subcommand_spec.cr | 21 +++++++++++++++++++++ src/cli.cr | 4 ++-- 2 files changed, 23 insertions(+), 2 deletions(-) create mode 100644 spec/integration/subcommand_spec.cr diff --git a/spec/integration/subcommand_spec.cr b/spec/integration/subcommand_spec.cr new file mode 100644 index 00000000..3ec44e4a --- /dev/null +++ b/spec/integration/subcommand_spec.cr @@ -0,0 +1,21 @@ +require "./spec_helper" + +describe "subcommand" do + it "forwards all arguments to subcommand" do + create_shard("dummy", "0.1.0") + create_executable "dummy", "bin/shards-dummy", %(print "args: "; print ARGV) + + with_path(git_path("dummy/bin")) do + output = run("shards dummy --no-color --verbose --unknown other argument") + output.should contain(%(args: ["--no-color", "--verbose", "--unknown", "other", "argument"])) + end + end +end + +private def with_path(path) + old_path = ENV["PATH"] + ENV["PATH"] = "#{File.expand_path(path)}#{Process::PATH_DELIMITER}#{ENV["PATH"]}" + yield +ensure + ENV["PATH"] = old_path +end diff --git a/src/cli.cr b/src/cli.cr index 668c825e..b129a511 100644 --- a/src/cli.cr +++ b/src/cli.cr @@ -101,7 +101,7 @@ module Shards else program_name = "shards-#{args[0]}" if program_path = Process.find_executable(program_name) - run_shards_subcommand(program_path, args) + run_shards_subcommand(program_path, cli_options) else display_help_and_exit(opts) end @@ -119,7 +119,7 @@ module Shards {% else %} shards_opts = ENV.fetch("SHARDS_OPTS", "").split {% end %} - ARGV.concat(shards_opts) + ARGV.dup.concat(shards_opts) end def self.run_shards_subcommand(process_name, args) From 5a6dc6cb28973f22758532a76e4585cb13969cfc Mon Sep 17 00:00:00 2001 From: Luis Lavena Date: Mon, 20 May 2024 12:55:01 +0200 Subject: [PATCH 2/4] Fixes passing --help to subcommand Avoid OptionParser to short-circuit `--help` and return immediately by setting a flag for it and evaluating at the end of the processing of unknown options. This is only done for the CLI invocation and is not part of Shards module (as the help and usage options are only available in this context). --- spec/integration/subcommand_spec.cr | 10 ++++++++++ src/cli.cr | 8 +++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/spec/integration/subcommand_spec.cr b/spec/integration/subcommand_spec.cr index 3ec44e4a..766c6c51 100644 --- a/spec/integration/subcommand_spec.cr +++ b/spec/integration/subcommand_spec.cr @@ -10,6 +10,16 @@ describe "subcommand" do output.should contain(%(args: ["--no-color", "--verbose", "--unknown", "other", "argument"])) end end + + it "correctly forwards '--help' option to subcommand" do + create_shard("dummy", "0.1.0") + create_executable "dummy", "bin/shards-dummy", %(print "args: "; print ARGV) + + with_path(git_path("dummy/bin")) do + output = run("shards dummy --help") + output.should contain(%(args: ["--help"])) + end + end end private def with_path(path) diff --git a/src/cli.cr b/src/cli.cr index b129a511..3e92c42a 100644 --- a/src/cli.cr +++ b/src/cli.cr @@ -2,6 +2,8 @@ require "option_parser" require "./commands/*" module Shards + class_property? display_help : Bool = false + def self.display_help_and_exit(opts) puts <<-HELP shards [...] [] @@ -53,7 +55,7 @@ module Shards opts.on("--ignore-crystal-version", "Has no effect. Kept for compatibility, to be removed in the future.") { } opts.on("-v", "--verbose", "Increase the log verbosity, printing all debug statements.") { self.set_debug_log_level } opts.on("-q", "--quiet", "Decrease the log verbosity, printing only warnings and errors.") { self.set_warning_log_level } - opts.on("-h", "--help", "Print usage synopsis.") { self.display_help_and_exit(opts) } + opts.on("-h", "--help", "Print usage synopsis.") { self.display_help = true } opts.unknown_args do |args, options| case args[0]? || DEFAULT_COMMAND @@ -107,6 +109,10 @@ module Shards end end + if display_help? + display_help_and_exit(opts) + end + exit end end From 49ac7671e0e335f94eedb2f4ec22183688c1f7cd Mon Sep 17 00:00:00 2001 From: Luis Lavena Date: Mon, 20 May 2024 22:53:28 +0200 Subject: [PATCH 3/4] Only use dummy executable on Windows Follow the pattern used in other tests and use simple `sh` script on non-Windows platforms. --- spec/integration/subcommand_spec.cr | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/spec/integration/subcommand_spec.cr b/spec/integration/subcommand_spec.cr index 766c6c51..89226a04 100644 --- a/spec/integration/subcommand_spec.cr +++ b/spec/integration/subcommand_spec.cr @@ -3,21 +3,31 @@ require "./spec_helper" describe "subcommand" do it "forwards all arguments to subcommand" do create_shard("dummy", "0.1.0") - create_executable "dummy", "bin/shards-dummy", %(print "args: "; print ARGV) + {% if flag?(:win32) %} + create_executable "dummy", "bin/shards-dummy", %(print ARGV.join(" ")) + {% else %} + path = create_file("dummy", "bin/shards-dummy", "#!/bin/sh\necho $@\n") + File.chmod(path, 0o755) + {% end %} with_path(git_path("dummy/bin")) do output = run("shards dummy --no-color --verbose --unknown other argument") - output.should contain(%(args: ["--no-color", "--verbose", "--unknown", "other", "argument"])) + output.should contain(%(--no-color --verbose --unknown other argument)) end end it "correctly forwards '--help' option to subcommand" do create_shard("dummy", "0.1.0") - create_executable "dummy", "bin/shards-dummy", %(print "args: "; print ARGV) + {% if flag?(:win32) %} + create_executable "dummy", "bin/shards-dummy", %(print ARGV.join(" ")) + {% else %} + path = create_file("dummy", "bin/shards-dummy", "#!/bin/sh\necho $@\n") + File.chmod(path, 0o755) + {% end %} with_path(git_path("dummy/bin")) do output = run("shards dummy --help") - output.should contain(%(args: ["--help"])) + output.should contain(%(--help)) end end end From 0375af5a90fe9daf47e07e58d35b46ee68ad56fa Mon Sep 17 00:00:00 2001 From: Luis Lavena Date: Tue, 21 May 2024 22:54:53 +0200 Subject: [PATCH 4/4] Avoid usage of class properties to handle help behavior Revert the usage of class property introduced in 5a6dc6cb28973f22758532a76e4585cb13969cfc and leverage instead on a pure instance variable in the context of `Shards.run`. --- src/cli.cr | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cli.cr b/src/cli.cr index 3e92c42a..beb8ad24 100644 --- a/src/cli.cr +++ b/src/cli.cr @@ -2,8 +2,6 @@ require "option_parser" require "./commands/*" module Shards - class_property? display_help : Bool = false - def self.display_help_and_exit(opts) puts <<-HELP shards [...] [] @@ -28,6 +26,8 @@ module Shards end def self.run + display_help = false + OptionParser.parse(cli_options) do |opts| path = Dir.current @@ -55,7 +55,7 @@ module Shards opts.on("--ignore-crystal-version", "Has no effect. Kept for compatibility, to be removed in the future.") { } opts.on("-v", "--verbose", "Increase the log verbosity, printing all debug statements.") { self.set_debug_log_level } opts.on("-q", "--quiet", "Decrease the log verbosity, printing only warnings and errors.") { self.set_warning_log_level } - opts.on("-h", "--help", "Print usage synopsis.") { self.display_help = true } + opts.on("-h", "--help", "Print usage synopsis.") { display_help = true } opts.unknown_args do |args, options| case args[0]? || DEFAULT_COMMAND @@ -109,7 +109,7 @@ module Shards end end - if display_help? + if display_help display_help_and_exit(opts) end