-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 combining wt args and "wt new-tab" args in implicit context #8315
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,50 +69,29 @@ int AppCommandlineArgs::ParseCommand(const Commandline& command) | |
{ | ||
throw CLI::CallForHelp(); | ||
} | ||
// Clear the parser's internal state | ||
_app.clear(); | ||
|
||
// attempt to parse the commandline | ||
// attempt to parse the commandline prefix of the form [options][subcommand] | ||
_app.parse(args); | ||
auto remainingParams = _app.remaining_size(); | ||
|
||
// If we parsed the commandline, and _no_ subcommands were provided, try | ||
// parsing again as a "new-tab" command. | ||
|
||
// parse the remaining suffix as a "new-tab" command. | ||
if (_noCommandsProvided()) | ||
{ | ||
_newTabCommand.subcommand->clear(); | ||
_newTabCommand.subcommand->parse(args); | ||
remainingParams = _newTabCommand.subcommand->remaining_size(); | ||
} | ||
|
||
// if after parsing the prefix and (optionally) the implicit tab subcommand | ||
// we still have unparsed parameters we need to fail | ||
if (remainingParams > 0) | ||
{ | ||
throw CLI::ExtrasError(args); | ||
} | ||
} | ||
catch (const CLI::CallForHelp& e) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are we losing support for CallForHelp? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From my understanding the CallForHelp was here only to prevent implicit subcommand parsing (the second catch clause). Now we don't have this catch clause, thus we can handle all exceptions the same way by calling _handleExit, that will still show the relevant UI. |
||
{ | ||
return _handleExit(_app, e); | ||
} | ||
catch (const CLI::ParseError& e) | ||
{ | ||
// If we parsed the commandline, and _no_ subcommands were provided, try | ||
// parsing again as a "new-tab" command. | ||
if (_noCommandsProvided()) | ||
{ | ||
try | ||
{ | ||
// CLI11 mutated the original vector the first time it tried to | ||
// parse the args. Reconstruct it the way CLI11 wants here. | ||
// "See above for why it's begin() + 1" | ||
std::vector<std::string> args{ command.Args().begin() + 1, command.Args().end() }; | ||
std::reverse(args.begin(), args.end()); | ||
_newTabCommand.subcommand->clear(); | ||
_newTabCommand.subcommand->parse(args); | ||
} | ||
catch (const CLI::ParseError& e) | ||
{ | ||
return _handleExit(*_newTabCommand.subcommand, e); | ||
} | ||
} | ||
else | ||
{ | ||
return _handleExit(_app, e); | ||
} | ||
return _handleExit(_app, e); | ||
} | ||
return 0; | ||
} | ||
|
@@ -157,6 +136,15 @@ int AppCommandlineArgs::_handleExit(const CLI::App& command, const CLI::Error& e | |
// - <none> | ||
void AppCommandlineArgs::_buildParser() | ||
{ | ||
// We define or parser as a prefix command, to support "implicit new tab subcommand" scenario. | ||
// In this scenario we will try to parse the prefix that contains parameters like launch mode, | ||
// but will not encounter an explicit command. | ||
// Instead we will encounter an argument that doesn't belong to the prefix indicating the prefix is over. | ||
// Then we will try to parse the remaining arguments as a new tab subcommand. | ||
// E.g., for "wt.exe -M -d c:/", we will use -M for the launch mode, but once we will encounter -d | ||
// we will know that the prefix is over and try to handle the suffix as a new tab subcommand | ||
_app.prefix_command(); | ||
|
||
// -v,--version: Displays version info | ||
auto versionCallback = [this](int64_t /*count*/) { | ||
// Set our message to display the application name and the current version. | ||
|
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.
who catches this? Does it end up down on line 92 as a ParseError?
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.
Yes. It is caught there as a parse error. Implemented this to allow future handling. I can handle it immediately if you prefer.