-
Notifications
You must be signed in to change notification settings - Fork 552
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix subcommand method help #464
Conversation
@akm I have the very same issue and it's fixed by this patch 👍 |
@sferik Would it be possible to get this merged? |
@@ -187,7 +187,7 @@ def with_args(*args) | |||
begin | |||
$thor_runner = false | |||
help_output = capture(:stdout) { BoringVendorProvidedCLI.start(%w[exciting]) } | |||
expect(help_output).to include("thor exciting_plugin_c_l_i fireworks") | |||
expect(help_output).to include("thor exciting fireworks") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should the behavior change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because ExcitingPluginCLI is registered with subcommand_name "exciting" by the following code:
BoringVendorProvidedCLI.register(
ExcitingPluginCLI,
"exciting",
"do exciting things",
"Various non-boring actions")
fix conflict |
@@ -238,6 +238,9 @@ def subcommand(subcommand, subcommand_class) | |||
args.unshift("help") if opts.include? "--help" or opts.include? "-h" | |||
invoke subcommand_class, args, opts, :invoked_via_subcommand => true, :class_options => options | |||
end | |||
subcommand_class.commands.each do |meth, command| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the downside of simply setting the namespace on the class to the subcommand name by default ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@doudou Could you explain what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workaround I found (and that, it seems, is generally used) is to explicitely set the subcommand's namespace, as
class MainTest < Thor
namespace 'test'
end
I was wondering what are the upside/downside of your solution vs. doing
subcommand_class.namespace subcommand
in #subcommand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@doudou thank you for your comment.
Did you mean like this?
#!/usr/bin/env ruby
require 'thor'
require 'thor/version'
puts "Thor::VERSION: #{Thor::VERSION}"
module SubcommandTest1
class Child1 < Thor
namespace "child1"
desc "foo NAME", "Fooo"
def foo(name)
puts "#{name} was given"
end
end
class Parent < Thor
desc "child1", "child1 description"
subcommand "child1", Child1
end
end
SubcommandTest1::Parent.start(ARGV)
Save this file as thortest
and chmod 755 thortest
.
But I think it behaves a little strange:
$ ./thortest
Thor::VERSION: 0.19.1
Commands:
thortest child1 # child1 description
thortest help [COMMAND] # Describe available commands or one specific command
$ ./thortest child1
Thor::VERSION: 0.19.1
Commands:
thortest child1 foo NAME # Fooo
thortest child1 help [COMMAND] # Describe subcommands or one specific subc...
$ ./thortest child1 foo
Thor::VERSION: 0.19.1
ERROR: "thortest foo" was called with no arguments
Usage: "thortest foo NAME"
The namespace "child1" has gone from the last message.
What should I do to see the message like this?
$ ./thortest child1 foo
ERROR: "thortest child1 foo" was called with no arguments
Usage: "thortest child1 foo NAME"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess one would have to fix Base#handle_argument_error to add namespace support
@akm: can you comment on closing this ? Did you push another fix for the same issue ? |
@doudou Sorry. I've just mistaken, so re-opened this PR. |
@akm Thank you for this patch! |
@sferik Thank you for merging this! |
If these classes are defined,
SubcommandTest1::Parent.start(%w[child1 foo])
puts messages to $stderron current master
on this pull request branch