[SHACK-136] Ensure help works consistently #79
Conversation
marcparadise
force-pushed
the
SHACK-136/help-usage-consistency
branch
4 times, most recently
from
87e27b5
to
30bf9a5
Compare
marcparadise
force-pushed
the
SHACK-136/help-usage-consistency
branch
from
30bf9a5
to
893f6c6
Compare
marcparadise
changed the title
| show_help | ||
| elsif params.include?("-v") || params.include?("--version") |
There was a problem hiding this comment.
This shouldn't be necessary since we force the version command at the cli.rb level - will verify and remove
marcparadise
force-pushed
the
SHACK-136/help-usage-consistency
branch
from
893f6c6
to
dc19c6c
Compare
|
Fixed failing rubocop |
There was a problem hiding this comment.
Group review pass comments w/ @cheeseplus @robbkidd @tyler-ball
| Converge the specified <TARGET> with the single <RESOURCE>. [PROPERTIES] | ||
| should be specified as key=value. EG: | ||
| chef target converge <TARGET> <RESOURCE> <RESOURCE_NAME> [PROPERTIES] [FLAGS] | ||
|
|
There was a problem hiding this comment.
missing target converge for recipe
| # rescue OptionParser::InvalidOption, OptionParser::MissingArgument | ||
| # raise Shak::OptionParserError.new(opt_parser.to_s) | ||
| Log.debug "Completed #{@command_spec.qualified_name} command without exception" | ||
| # rescue OptionParser::InvalidOption, OptionParser::MissingArgument |
There was a problem hiding this comment.
add TODO for follow-up on commented out code
There was a problem hiding this comment.
Comment removed - we handle the error fine now
| end | ||
|
|
||
| def run(params) | ||
| raise NotImplementedError.new | ||
| show_help |
There was a problem hiding this comment.
I don't think this is necessary - verify that that 'chef help' with no args works without it.
There was a problem hiding this comment.
In my tests, we did still need this
| private | ||
|
|
||
| def show_version | ||
| UI::Terminal.output(ChefWorkstation::VERSION) |
There was a problem hiding this comment.
I don't think we need this method - it should all be handled by the new version.rb command
| # In a future pass, we may want to actually structure it that way | ||
| # such that a "Base' instance named 'chef' is the root command. | ||
| if self.class == Base | ||
| ChefWorkstation.commands_map.command_specs |
There was a problem hiding this comment.
Verify that we don't actually have @command_spec even for Base.
There was a problem hiding this comment.
We do have @command_spec for the hidden base because we manually populate that in builtin_commands.rb. I'm definitely thinking we want to restructure that have a root command whose subcommands are all the subcommands to the chef command
There was a problem hiding this comment.
I also think we want two pieces of functionality for help - First, we would have a help command that is a child of the chef command and would show the top level help. Second, I think we want some shared logic that all commands have for showing their help text. In the case of the root command it could shortcut this by still calling the help subcommand
| @@ -19,7 +19,7 @@ | |||
| require "chef-workstation/command/target/converge" | |||
|
|
|||
| RSpec.describe ChefWorkstation::Command::Target::Converge do | |||
| let(:cmd_spec) { instance_double(ChefWorkstation::CommandsMap::CommandSpec) } | |||
| let(:cmd_spec) { ChefWorkstation::CommandsMap::CommandSpec.new } | |||
There was a problem hiding this comment.
revert this line - should work with the original
tyler-ball
force-pushed
the
SHACK-136/help-usage-consistency
branch
from
dc19c6c
to
645c4a9
Compare
There was a problem hiding this comment.
I'm 👍 on this but it really makes the case for fixing the Commands Map. What we have now is an improvement, but there are still a lot of weird inconsistencies. I think making a root command will help with that. We know we are going to have to deal with some wonkyness (-v being automatically translated to the version command, having both a help command and every subcommand able to show its own help text, etc.) but I think we can encapsulate those weird side cases better than we are now.
I also think we need some integration/functional (possibly via Aruba) testing that covers all the different help text formats we specified in the card. Maybe we do that as another PR, but leave it in the scope of the current card?
Ensure help works consistently. If help is visible anywhere in the command as --help, -h, or help this will treat it as the 'help' flag and ensure that the lowest level subcommand (before the 'help' word was encountered) is the one that renders help. Version has been moved to its own command, but we have retained a '-v' flag for easy-of-use.
tyler-ball
force-pushed
the
SHACK-136/help-usage-consistency
branch
from
645c4a9
to
481c3e6
Compare
|
Yeah, +1 on fixing command map. I see you have a card already. I also had created a card that's in flight now SHACK-155 - which covers adding CLI integration tests. |
Ensure help works consistently.
If help is visible anywhere in the command as --help, -h, or help
this will treat it as the 'help' flag and ensure that the lowest
level subcommand (before the 'help' word was encountered) is the one
that renders help.
Version has been moved to its own command, but we have retained a '-v'
flag for easy-of-use.
Signed-off-by: Marc A. Paradise marc.paradise@gmail.com