rails / thor

Thor is a toolkit for building powerful command-line interfaces.
http://whatisthor.com/
MIT License
5.12k stars 553 forks source link

Positional arguments with colons are incompatible with hash method options #881

Closed aliismayilov closed 2 months ago

aliismayilov commented 2 months ago

Not sure if applicable for issues: 🌈

When there is a method option as hash and one of the positional arguments also includes a colon, then the command parser fails. To demonstrate:

#!/usr/bin/env ruby

require "thor"

class Kamal < Thor
  def self.exit_on_failure? = true

  desc "exec command", "an exec task"
  method_option :env, type: :hash
  def exec(command)
    puts "these are the options: #{options}"
    puts "this is the command: #{command}"
  end
end

Kamal.start

# ./kamal.rb exec --env='STATEMENT_TIMEOUT:0' 'bin/rails db:vacuum' => fails
# ./kamal.rb exec 'bin/rails db:vacuum' --env='STATEMENT_TIMEOUT:0' => works
# ./kamal.rb exec --env='STATEMENT_TIMEOUT:0' 'bin/rails vacuum' => works
# ./kamal.rb exec 'bin/rails vacuum' --env='STATEMENT_TIMEOUT:0' => works

Failure looks like this:

$ ./kamal.rb exec --env='STATEMENT_TIMEOUT:0' 'bin/rails db:vacuum'
ERROR: "kamal.rb exec" was called with no arguments
Usage: "kamal.rb exec command"

related: https://github.com/basecamp/kamal/issues/800

aliismayilov commented 2 months ago

Not sure if this is solvable given the current syntax expectations. According to the docs the following is valid and works:

❯ ./kamal.rb exec db:migrate --env STATEMENT_TIMEOUT:0 foo:bar
these are the options: {"env"=>{"STATEMENT_TIMEOUT"=>"0", "foo"=>"bar"}}
this is the command: db:migrate

On the other hand, moving db:migrate to the end will always yield ambiguous result. I don't see why thor shouldn't parse db:migrate in the following example as part of the --env hash.

❯ ./kamal.rb exec --env STATEMENT_TIMEOUT:0 foo:bar db:migrate

One could argue that the positional arguments should always be extracted, but that also won't always work, since positional arguments can be optional.