Issue 797473002: Add a CommandRunner class for dispatching commands to args.

nweiz

nweiz@google.com changed reviewers: + rnystrom@google.com

6 years ago (2014-12-11 01:15:17 UTC) #1

Bob Nystrom

https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md File pkg/args/README.md (right): https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md#newcode215 pkg/args/README.md:215: // The [name] and [description] fields must be set ...

6 years ago (2014-12-11 20:25:31 UTC) #3

nweiz

https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md File pkg/args/README.md (right): https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md#newcode215 pkg/args/README.md:215: // The [name] and [description] fields must be set ...

6 years ago (2014-12-11 23:55:24 UTC) #5

Sean Eagan

On 2014/12/11 01:15:18, nweiz wrote: Thanks for the cc! Why not a separate package? Why ...

6 years ago (2014-12-12 17:50:12 UTC) #6

On 2014/12/11 01:15:18, nweiz wrote:

Thanks for the cc!

Why not a separate package?  Why should this approach be blessed by `args` when
there is at least one other existing approach:

  http://pub.dartlang.org/packages/unscripted

which (in my completely biased opinion) has many advantages:

* You can reuse your dart API design skills to define CLIs.
* `main` is always just a one-liner wrapping a normal dart method or class(es),
which can be progressively enhanced with metadata.
* You can visualize the command-line interface by looking at the dart code,
especially taking syntax highlighting and dart style guide compliance into
account.
* Removes the need to extract args from an ArgResults into local variables, they
are injected automatically.
* Safely and easily refactor command, option, and flag names.
* Better utilize static analysis to find typos.
* Much more DRY.
* Easily unit test the entire cli script, since it is just a dart method or
class which you can inject mock parameters into.
* You can often reuse the dart method or class that defines your cli as a
programmatic API.

I'm sure `command_runner` has advantages too, but I don't think this problem
should be solved by the `args` package, at least until a clear winner emerges.

General thoughts on the API:

We already have `ArgParser` for defining the *interface* for commands and
subcommands.  Some stuff in CommandRunner and Command should be added there
instead.

Unscripted has an internal `Usage` type which defines the CLI interface,
wrapping an ArgParser:

  https://github.com/seaneagan/unscripted/blob/master/lib/src/usage.dart

...adding features, and trying to improve the API, for example:

* Automatically adds help flags and commands.
* Adds a description field.
* There is a separate `Flag` type which extends `Option`.
* `addOption` takes an `Option` or `Flag` instead of several arguments.
* Supports individual positional arguments and a rest argument with
`addPotional(Positional positional)`.
* Supports parsing/validation/transformation of options, flags, and positionals,
throws appropriate UsageErrors automatically.
* Allows commands to be hidden.

I would love it if `ArgParser` made some of these changes.

(the same file also has an attempt at an improved version of ArgResults, but
thats a separate discussion)

But I think Command/CommandRunner should focus on defining the implementation of
commands, and dispatching to the appropriate command.  I agree with Bob that
Command and CommandRunner should share a common base class or one should inherit
from the other, just like ArgParser defined the interface for the top-level
command, and for subcommands.

Sean Eagan

https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/command_runner.dart File pkg/args/lib/src/command_runner.dart (right): https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/command_runner.dart#newcode70 pkg/args/lib/src/command_runner.dart:70: CommandRunner(this.executableName, this.description) { The executable name should rarely if ...

6 years ago (2014-12-12 17:50:32 UTC) #8

https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/comm...
File pkg/args/lib/src/command_runner.dart (right):

https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/comm...
pkg/args/lib/src/command_runner.dart:70: CommandRunner(this.executableName,
this.description) {
The executable name should rarely if ever need to be explicitly set, as it can
be deduced from dart:io's `Platform.script`.  So this parameter should be
optional if it's included.

Description should be optional too, if nothing else so that `description: `
shows up at the call site to make it clear what the String is for.

https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/comm...
pkg/args/lib/src/command_runner.dart:112: } on FormatException catch (error) {
Why not just fix ArgParser.parse to throw UsageError instead of FormatException?

https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/comm...
pkg/args/lib/src/command_runner.dart:299: final takesArguments = true;
This is too coarse grained.  Unscripted allows adding individual positional
arguments, and also a trailing "rest" argument, with the same sort of metadata
as options and flags.  It can then validate the right number and type of
positional args as well, and make those easily accessible.

The default should be no positional args allowed.  The cli author should
explicitly define their positionals just as they do with named arguments
(options and flags).

https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/comm...
pkg/args/lib/src/command_runner.dart:307: final aliases = const <String>[];
If adding this, it ought to be exposed on ArgParser directly, it's just as
meaningful there.

Personally I think aliases are an anti-pattern.  Two-ways to do the same thing,
but they are pretty commonly used.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
File pkg/args/lib/command_runner.dart (right):

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:23: /// Used for error reporting.
also used for usage text.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:33: String get usageTemplate =>
"$executableName [command] <arguments>";
invocationTemplate?
invocationHelp?

Since `usage` refers to the entire help text, `usageTemplate` sounds like a
template for the entire help text.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:54: String get _usageWithoutDescription {
Why is this necessary in the first place? Why not just include the description
in the usage error message?

If it is needed, the impl below assumes that the overridden usage starts with
'$description\n\n`, which may not be true, so why not make it true by only
allowing to override the `usageWithoutDescription` (with a better name though)
and `description` separately` and just always add the `\n\n` (or make that
overridable too?

In unscripted I have all the usage formatting stuff separated into a
`UsageFormatter` class which formats `Usage`s which I haven't exposed yet, but
when I do, the user will be allowed to pass that in to override the formatting. 
It provides extension points, instead of an all or nothing overriding.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:72: CommandRunner(this.executableName,
this.description) {
executableName is almost always derivable from dart:io's `Platform.script`, so
it should at least be an optional argument that defaults to being derived.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:75: addCommand(new HelpCommand());
This should only be added when the first sub-command is added.  If there are
no-subcommands, it isn't needed.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:232: ArgResults get globalOptions =>
_globalOptions;
It's weird to define [options] and [globalOptions] as fields instead of just
passing them to [run].  I guess that's to avoid having to define them as
parameters when you don't need them.  That is only a problem because dart
doesn't allow passing extra arguments.  It also doesn't give you everything you
might need if you have 2 or more levels of sub-commands.

Why not add a `parent` field, and possibly a `root`, `top`, or `global` field to
ArgResults and then just pass the leaf ArgResults to [run].

Also, I don't like calling them `options`, it should be something like
`argResults` since `ArgResults` already supports things besides options, and
hopefully will have support for positionals in the future as well.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:283: /// Whether or not this command should
appear in help listings.
appear in -> be hidden from

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:289: /// commands as long as any of their leaf
commands are visible.
true -> false

or "leaf commands are always visible", "branch commands are visible as long as"

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:290: bool get hidden {
Why not add this directly to ArgParser?

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:306: final takesArguments = true;
This is too coarse.  Why should individual positional arguments get any less
detail than options and flags?  Unscripted allows defining both individual
positional arguments, and a trailing rest argument.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:314: final aliases = const <String>[];
How about an AliasCommand for this:

command.addSubcommand(canonicalCommand);
command.addSubcommand(new AliasCommand('alias', canonicalCommand));

Then you could override some details of the aliases by mixing into or extending
AliasCommand.  For example, maybe you want to run analytics on which commands
are being used, including the aliases.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:343: void printUsage() => print(usage);
I would think "to control how output is displayed or integrate with a logging
system" would be best done for all output of a command, not just the help
output.  There is already `usage` to customize the help content.

Same for CommandRunner.printUsage.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:346: void usageError(String message) =>
usageException?

Bob Nystrom

https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md File pkg/args/README.md (right): https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md#newcode215 pkg/args/README.md:215: // The [name] and [description] fields must be set ...

6 years ago (2014-12-12 18:13:22 UTC) #9

https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md
File pkg/args/README.md (right):

https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md#newcode215
pkg/args/README.md:215: // The [name] and [description] fields must be set by
every subclass.
On 2014/12/11 23:55:24, nweiz wrote:
> Changed "set" to "defined".

That still isn't correct. Subclasses don't have to define a field. They have to
define a getter, and a field is just one way to do that.

> I still find defining constant-valued getters to be perverse.

I still find defining constant-valued instance fields to be even more perverse.
:)

From the perspective of a caller, all it cares is that any instance of Command
responds to .name. It doesn't care if that's a getter whose body is a literal,
some calculation, or a field. From the caller's perspective, it's just an
argument-less member invocation.

If you have some class that implements that, and every instance of the class
returns the same value, then a getter that returns that value seems like the
natural way to express that to me. It makes it clear to the reader that all
instances of the class have the same behavior (since it's a getter) and that
they all return the same value (since the body is just a literal).

Fields are a property of each individual instance, but here, "my name is
'commit'" is true for all instances of the CommitCommand class. If you have some
concept that you want to share across all instances of a class, a member is the
natural way to express it.

It's also faster and uses less memory.

https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md#newcode232
pkg/args/README.md:232: command with subcommands can't run its own code, so
[run][] doesn't need to be
On 2014/12/11 23:55:24, nweiz wrote:
> On 2014/12/11 20:25:30, Bob Nystrom wrote:
> > Why can't parent commands also be run?
> 
> Because the pub command runner didn't support it and it wasn't trivial to add.
> In particular, it's tricky to detect the difference between a runnable branch
> command and a non-runnable one. I think it's a tractable problem, but not one
I
> wanted to deal with in this CL.

This CL isn't urgent, though, is it?

I could be wrong but my intuition is that allowing branch commands to be run
will simplify the API because it will let you get rid of some duplication
between CommandRunner and Command.

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
File pkg/args/lib/src/command_runner.dart (right):

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
pkg/args/lib/src/command_runner.dart:55: return
usage.replaceFirst("$description\n\n", "");
On 2014/12/11 23:55:24, nweiz wrote:
> On 2014/12/11 20:25:30, Bob Nystrom wrote:
> > This feels super sketchy to me. What's the use case for overriding `usage`?
> 
> Usually extra bottom-matter. Pub does it to add links to the documentation
site,
> for instance.

That was my guess. How about just making a "footer" getter that can be
overridden then?

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
pkg/args/lib/src/command_runner.dart:339: throw new UsageException(message,
_usageWithoutDescription);
On 2014/12/11 23:55:24, nweiz wrote:
> On 2014/12/11 20:25:30, Bob Nystrom wrote:
> > These methods are pretty similar to ones in CommandRunner. Maybe make
> > CommandRunner a subclass of Command?
> 
> I think that muddies the semantic distinction too much. The executable is not
> itself a command.

You think so? It has some extra functionality, but I think almost everything
that's true of commands is also true of it:

1. It can have (sub)commands.
2. It can have options.
3. It has an argParser.
4. It can raise usage errors.

I wonder if users will want to be able to have CommandRunners that are also used
as Commands for some other metacommand. Imagine some package that exposes a big
executable with a few different commands. But it also defines top-level
executables for a few of the more common commands so they can be invoked
directly. It would be nice if those could reuse a lot of the same command code.

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
pkg/args/lib/src/command_runner.dart:356: // Show the commands alphabetically.
On 2014/12/11 23:55:24, nweiz wrote:
> On 2014/12/11 20:25:30, Bob Nystrom wrote:
> > Do you think users will want to be able to control this order by controlling
> the
> > order they add commands?
> 
> I think it's more likely that the user will want consistent ordering despite
> commands being added in disparate parts of the program.
> 
> If we get user demand for custom ordering, we can add support later. For now I
> think it's better to focus on providing good defaults rather than trying to
make
> it as extensible as possible.

SGTM.

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/help_comman...
File pkg/args/lib/src/help_command.dart (right):

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/help_comman...
pkg/args/lib/src/help_command.dart:13: final name = "help";
On 2014/12/11 23:55:24, nweiz wrote:
> On 2014/12/11 20:25:31, Bob Nystrom wrote:
> > Why not make this a getter too?
> 
> I don't like constant-valued getters when fields will do. [description] and
> [usage] are only getters because their values are dynamic.

I know you feel that way, but I still don't understand it. Getters are
semantically clearer, faster, and use less memory. What's not to love?

Sean Eagan

On 2014/12/12 17:50:32, Sean Eagan wrote: > https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/command_runner.dart > File pkg/args/lib/src/command_runner.dart (right): > > https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/command_runner.dart#newcode70 ...

6 years ago (2014-12-12 18:24:56 UTC) #10

On 2014/12/12 17:50:32, Sean Eagan wrote:
>
https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/comm...
> File pkg/args/lib/src/command_runner.dart (right):
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/comm...
> pkg/args/lib/src/command_runner.dart:70: CommandRunner(this.executableName,
> this.description) {
> The executable name should rarely if ever need to be explicitly set, as it can
> be deduced from dart:io's `Platform.script`.  So this parameter should be
> optional if it's included.
> 
> Description should be optional too, if nothing else so that `description: `
> shows up at the call site to make it clear what the String is for.
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/comm...
> pkg/args/lib/src/command_runner.dart:112: } on FormatException catch (error) {
> Why not just fix ArgParser.parse to throw UsageError instead of
FormatException?
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/comm...
> pkg/args/lib/src/command_runner.dart:299: final takesArguments = true;
> This is too coarse grained.  Unscripted allows adding individual positional
> arguments, and also a trailing "rest" argument, with the same sort of metadata
> as options and flags.  It can then validate the right number and type of
> positional args as well, and make those easily accessible.
> 
> The default should be no positional args allowed.  The cli author should
> explicitly define their positionals just as they do with named arguments
> (options and flags).
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/1/pkg/args/lib/src/comm...
> pkg/args/lib/src/command_runner.dart:307: final aliases = const <String>[];
> If adding this, it ought to be exposed on ArgParser directly, it's just as
> meaningful there.
> 
> Personally I think aliases are an anti-pattern.  Two-ways to do the same
thing,
> but they are pretty commonly used.
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> File pkg/args/lib/command_runner.dart (right):
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> pkg/args/lib/command_runner.dart:23: /// Used for error reporting.
> also used for usage text.
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> pkg/args/lib/command_runner.dart:33: String get usageTemplate =>
> "$executableName [command] <arguments>";
> invocationTemplate?
> invocationHelp?
> 
> Since `usage` refers to the entire help text, `usageTemplate` sounds like a
> template for the entire help text.
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> pkg/args/lib/command_runner.dart:54: String get _usageWithoutDescription {
> Why is this necessary in the first place? Why not just include the description
> in the usage error message?
> 
> If it is needed, the impl below assumes that the overridden usage starts with
> '$description\n\n`, which may not be true, so why not make it true by only
> allowing to override the `usageWithoutDescription` (with a better name though)
> and `description` separately` and just always add the `\n\n` (or make that
> overridable too?
> 
> In unscripted I have all the usage formatting stuff separated into a
> `UsageFormatter` class which formats `Usage`s which I haven't exposed yet, but
> when I do, the user will be allowed to pass that in to override the
formatting. 
> It provides extension points, instead of an all or nothing overriding.
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> pkg/args/lib/command_runner.dart:72: CommandRunner(this.executableName,
> this.description) {
> executableName is almost always derivable from dart:io's `Platform.script`, so
> it should at least be an optional argument that defaults to being derived.
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> pkg/args/lib/command_runner.dart:75: addCommand(new HelpCommand());
> This should only be added when the first sub-command is added.  If there are
> no-subcommands, it isn't needed.
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> pkg/args/lib/command_runner.dart:232: ArgResults get globalOptions =>
> _globalOptions;
> It's weird to define [options] and [globalOptions] as fields instead of just
> passing them to [run].  I guess that's to avoid having to define them as
> parameters when you don't need them.  That is only a problem because dart
> doesn't allow passing extra arguments.  It also doesn't give you everything
you
> might need if you have 2 or more levels of sub-commands.
> 
> Why not add a `parent` field, and possibly a `root`, `top`, or `global` field
to
> ArgResults and then just pass the leaf ArgResults to [run].
> 
> Also, I don't like calling them `options`, it should be something like
> `argResults` since `ArgResults` already supports things besides options, and
> hopefully will have support for positionals in the future as well.
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> pkg/args/lib/command_runner.dart:283: /// Whether or not this command should
> appear in help listings.
> appear in -> be hidden from
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> pkg/args/lib/command_runner.dart:289: /// commands as long as any of their
leaf
> commands are visible.
> true -> false
> 
> or "leaf commands are always visible", "branch commands are visible as long
as"
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> pkg/args/lib/command_runner.dart:290: bool get hidden {
> Why not add this directly to ArgParser?
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> pkg/args/lib/command_runner.dart:306: final takesArguments = true;
> This is too coarse.  Why should individual positional arguments get any less
> detail than options and flags?  Unscripted allows defining both individual
> positional arguments, and a trailing rest argument.
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> pkg/args/lib/command_runner.dart:314: final aliases = const <String>[];
> How about an AliasCommand for this:
> 
> command.addSubcommand(canonicalCommand);
> command.addSubcommand(new AliasCommand('alias', canonicalCommand));
> 
> Then you could override some details of the aliases by mixing into or
extending
> AliasCommand.  For example, maybe you want to run analytics on which commands
> are being used, including the aliases.
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> pkg/args/lib/command_runner.dart:343: void printUsage() => print(usage);
> I would think "to control how output is displayed or integrate with a logging
> system" would be best done for all output of a command, not just the help
> output.  There is already `usage` to customize the help content.
> 
> Same for CommandRunner.printUsage.
> 
>
https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
> pkg/args/lib/command_runner.dart:346: void usageError(String message) =>
> usageException?

Sorry for some duplicated comments, I created some before the last patch, and
couldn't find any way to navigate back to them, so I figured I needed to repost
them.

nweiz

On 2014/12/12 17:50:12, Sean Eagan wrote: > Thanks for the cc! > > Why not ...

6 years ago (2014-12-16 00:21:40 UTC) #11

On 2014/12/12 17:50:12, Sean Eagan wrote:
> Thanks for the cc!
> 
> Why not a separate package?  Why should this approach be blessed by `args`
when
> there is at least one other existing approach:
> 
>   http://pub.dartlang.org/packages/unscripted
> 
> which (in my completely biased opinion) has many advantages:
> 
> * You can reuse your dart API design skills to define CLIs.
> * `main` is always just a one-liner wrapping a normal dart method or
class(es),
> which can be progressively enhanced with metadata.
> * You can visualize the command-line interface by looking at the dart code,
> especially taking syntax highlighting and dart style guide compliance into
> account.
> * Removes the need to extract args from an ArgResults into local variables,
they
> are injected automatically.
> * Safely and easily refactor command, option, and flag names.
> * Better utilize static analysis to find typos.
> * Much more DRY.
> * Easily unit test the entire cli script, since it is just a dart method or
> class which you can inject mock parameters into.
> * You can often reuse the dart method or class that defines your cli as a
> programmatic API.
> 
> I'm sure `command_runner` has advantages too, but I don't think this problem
> should be solved by the `args` package, at least until a clear winner emerges.

The CommandRunner API is much more closely related to the existing args APIs
that unscripted's. It defines methods through explicit ArgParser calls and
provides explicit ArgResult objects for handling results. In a sense, it's the
most obvious means of command dispatching: it just chooses a class and runs a
method on it.

I think there's definitely room for more experimental APIs like unscripted, but
I think it also makes sense to include a straightforward default API in the main
args package. Command dispatch is an aspect of arg parsing.

> General thoughts on the API:
> 
> We already have `ArgParser` for defining the *interface* for commands and
> subcommands.  Some stuff in CommandRunner and Command should be added there
> instead.
>
> Unscripted has an internal `Usage` type which defines the CLI interface,
> wrapping an ArgParser:
> 
>   https://github.com/seaneagan/unscripted/blob/master/lib/src/usage.dart
> 
> ...adding features, and trying to improve the API, for example:
> 
> * Automatically adds help flags and commands.
> * Adds a description field.
> * There is a separate `Flag` type which extends `Option`.
> * `addOption` takes an `Option` or `Flag` instead of several arguments.
> * Supports individual positional arguments and a rest argument with
> `addPotional(Positional positional)`.
> * Supports parsing/validation/transformation of options, flags, and
positionals,
> throws appropriate UsageErrors automatically.
> * Allows commands to be hidden.
>
> I would love it if `ArgParser` made some of these changes.
>
> (the same file also has an attempt at an improved version of ArgResults, but
> thats a separate discussion)

These are mostly out-of-scope for this CL, but feel free to open a separate
issue against args to discuss them.

> But I think Command/CommandRunner should focus on defining the implementation
of
> commands, and dispatching to the appropriate command.

This is mostly what the commands do, but in order to generate full-fledged usage
information it's necessary to include a certain amount of documentary data in
them. I'm not sure how you're envisioning that moving to ArgParser, especially
since the user doesn't explicitly interact with it when adding a new command.

> I agree with Bob that
> Command and CommandRunner should share a common base class or one should
inherit
> from the other, just like ArgParser defined the interface for the top-level
> command, and for subcommands.

I don't like using subclassing, especially public subclassing, just for sharing
implementation. A CommandRunner is not a Command, and thus it shouldn't say that
it is via the inheritance hierarchy. I also don't think there's any particular
usage benefit to it doing so.

nweiz

https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md File pkg/args/README.md (right): https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md#newcode215 pkg/args/README.md:215: // The [name] and [description] fields must be set ...

6 years ago (2014-12-16 02:07:44 UTC) #13

https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md
File pkg/args/README.md (right):

https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md#newcode215
pkg/args/README.md:215: // The [name] and [description] fields must be set by
every subclass.
On 2014/12/12 18:13:22, Bob Nystrom wrote:
> On 2014/12/11 23:55:24, nweiz wrote:
> > Changed "set" to "defined".
> 
> That still isn't correct. Subclasses don't have to define a field. They have
to
> define a getter, and a field is just one way to do that.

Changed to "property" which matches dartdoc.

> > I still find defining constant-valued getters to be perverse.
> 
> I still find defining constant-valued instance fields to be even more
perverse.
> :)
> 
> From the perspective of a caller, all it cares is that any instance of Command
> responds to .name. It doesn't care if that's a getter whose body is a literal,
> some calculation, or a field. From the caller's perspective, it's just an
> argument-less member invocation.
> 
> If you have some class that implements that, and every instance of the class
> returns the same value, then a getter that returns that value seems like the
> natural way to express that to me. It makes it clear to the reader that all
> instances of the class have the same behavior (since it's a getter) and that
> they all return the same value (since the body is just a literal).
> 
> Fields are a property of each individual instance, but here, "my name is
> 'commit'" is true for all instances of the CommitCommand class. If you have
some
> concept that you want to share across all instances of a class, a member is
the
> natural way to express it.
>  
> It's also faster and uses less memory.

I'm tired of rehashing this argument over and over. I've stopped complaining
when you use a property style I don't like; will you do the same for me?

https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md#newcode232
pkg/args/README.md:232: command with subcommands can't run its own code, so
[run][] doesn't need to be
On 2014/12/12 18:13:22, Bob Nystrom wrote:
> On 2014/12/11 23:55:24, nweiz wrote:
> > On 2014/12/11 20:25:30, Bob Nystrom wrote:
> > > Why can't parent commands also be run?
> > 
> > Because the pub command runner didn't support it and it wasn't trivial to
add.
> > In particular, it's tricky to detect the difference between a runnable
branch
> > command and a non-runnable one. I think it's a tractable problem, but not
one
> I
> > wanted to deal with in this CL.
> 
> This CL isn't urgent, though, is it?

Well, it's blocking further work on the development server. But also this is a
minimal set of functionality for this feature, or at least minimal that supports
pub. Even if I were definitely planning to add that functionality, I'd want to
do it in a separate CL for minimality purposes.

> I could be wrong but my intuition is that allowing branch commands to be run
> will simplify the API because it will let you get rid of some duplication
> between CommandRunner and Command.

I don't follow your reasoning. The only API difference is that a branch command
which implements [run] will have it run; how does that help duplication?

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
File pkg/args/lib/src/command_runner.dart (right):

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
pkg/args/lib/src/command_runner.dart:55: return
usage.replaceFirst("$description\n\n", "");
On 2014/12/12 18:13:22, Bob Nystrom wrote:
> On 2014/12/11 23:55:24, nweiz wrote:
> > On 2014/12/11 20:25:30, Bob Nystrom wrote:
> > > This feels super sketchy to me. What's the use case for overriding
`usage`?
> > 
> > Usually extra bottom-matter. Pub does it to add links to the documentation
> site,
> > for instance.
> 
> That was my guess. How about just making a "footer" getter that can be
> overridden then?

Done.

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
pkg/args/lib/src/command_runner.dart:70: CommandRunner(this.executableName,
this.description) {
On 2014/12/12 17:50:31, Sean Eagan wrote:
> The executable name should rarely if ever need to be explicitly set, as it can
> be deduced from dart:io's `Platform.script`.  So this parameter should be
> optional if it's included.

args doesn't currently use dart:io. Adding an import now would be a breaking
change, and I don't think it's worth it just to make the executable name
optional. I'm also wary of depending too heavily on Platform.script.

> Description should be optional too, if nothing else so that `description: `
> shows up at the call site to make it clear what the String is for.

What would we do for the usage information if it's not provided?

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
pkg/args/lib/src/command_runner.dart:112: } on FormatException catch (error) {
On 2014/12/12 17:50:31, Sean Eagan wrote:
> Why not just fix ArgParser.parse to throw UsageError instead of
FormatException?

I'm not a fan of that for a few reasons:

* It would be a backwards-incompatible change unless we made UsageError extend
FormatException, which I don't think makes sense.
* I think FormatException still makes sense for parsing args. It's consistent
with other parse functions' exceptions.
* Even if we made that change, we'd still have to catch and rethrow here because
the arg parser doesn't know the runner's full usage string.

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
pkg/args/lib/src/command_runner.dart:299: final takesArguments = true;
On 2014/12/12 17:50:31, Sean Eagan wrote:
> This is too coarse grained.  Unscripted allows adding individual positional
> arguments, and also a trailing "rest" argument, with the same sort of metadata
> as options and flags.  It can then validate the right number and type of
> positional args as well, and make those easily accessible.

That would be a much broader change largely in ArgParser, out of scope of this
CL. If we did add it we could revise the use of this field, but I'll tell you
now that I'm not a big fan of that approach; positional arguments come in too
many different heterogeneous forms to effectively capture in an API.

> The default should be no positional args allowed.  The cli author should
> explicitly define their positionals just as they do with named arguments
> (options and flags).

I think it's most important that this match the behavior of ArgParser. In
general I think most commands also do want positional arguments. For example,
about 2/3s of pub's commands take positional arguments.

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
pkg/args/lib/src/command_runner.dart:307: final aliases = const <String>[];
On 2014/12/12 17:50:31, Sean Eagan wrote:
> If adding this, it ought to be exposed on ArgParser directly, it's just as
> meaningful there.

I'm open to adding this to ArgParser.addCommand, but not in this CL. The API
here is the same either way, since the user isn't calling that method directly.
Converting ArgParser.addCommand to take named arguments would also be a breaking
change.

> Personally I think aliases are an anti-pattern.  Two-ways to do the same
thing,
> but they are pretty commonly used.

They're important at least for backwards-compatibility; they're what allowed pub
to switch from "install" to "get", for example.

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
pkg/args/lib/src/command_runner.dart:339: throw new UsageException(message,
_usageWithoutDescription);
On 2014/12/12 18:13:22, Bob Nystrom wrote:
> On 2014/12/11 23:55:24, nweiz wrote:
> > On 2014/12/11 20:25:30, Bob Nystrom wrote:
> > > These methods are pretty similar to ones in CommandRunner. Maybe make
> > > CommandRunner a subclass of Command?
> > 
> > I think that muddies the semantic distinction too much. The executable is
not
> > itself a command.
> 
> You think so? It has some extra functionality, but I think almost everything
> that's true of commands is also true of it:
> 
> 1. It can have (sub)commands.
> 2. It can have options.
> 3. It has an argParser.
> 4. It can raise usage errors.

I don't think behavioral overlap alone is enough to indicate an inheritance
hierarchy. I also want either a semantic "is-a" relationship that matches the
hierarchy or a clear use-case for passing the subclass to a function that
declares the superclass.

> I wonder if users will want to be able to have CommandRunners that are also
used
> as Commands for some other metacommand. Imagine some package that exposes a
big
> executable with a few different commands. But it also defines top-level
> executables for a few of the more common commands so they can be invoked
> directly. It would be nice if those could reuse a lot of the same command
code.

I really don't think that's very likely. Git, which has more commands and
subcommands than probably anything, doesn't even do this.

The UI would also be pretty shaky if we ended up adding support for running
branch commands. The command runner would incorrectly format errors as though it
was the top-level executable.

https://codereview.chromium.org/797473002/diff/20001/pkg/args/lib/command_run...
File pkg/args/lib/command_runner.dart (right):

https://codereview.chromium.org/797473002/diff/20001/pkg/args/lib/command_run...
pkg/args/lib/command_runner.dart:23: /// Used for error reporting.
On 2014/12/12 17:50:32, Sean Eagan wrote:
> also used for usage text.

Done.

https://codereview.chromium.org/797473002/diff/20001/pkg/args/lib/command_run...
pkg/args/lib/command_runner.dart:33: String get usageTemplate =>
"$executableName [command] <arguments>";
On 2014/12/12 17:50:32, Sean Eagan wrote:
> invocationTemplate?
> invocationHelp?
> 
> Since `usage` refers to the entire help text, `usageTemplate` sounds like a
> template for the entire help text.

Yeah, I wasn't a big fan of this name. Pub had it as just "usage" but that's
obviously something else now.

I like just "invocation".

https://codereview.chromium.org/797473002/diff/20001/pkg/args/lib/command_run...
pkg/args/lib/command_runner.dart:54: String get _usageWithoutDescription {
On 2014/12/12 17:50:32, Sean Eagan wrote:
> Why is this necessary in the first place? Why not just include the description
> in the usage error message?

This was following pub's behavior, but I think the user experience is nice. An
error message of the form "here's the error, and the usage" looks a lot better
than "here's the error and the overview of the command/executable and the
usage". A usage error may well come from misunderstanding the usage, but it's
very unlikely that it will come from having no idea what the command is even
for, so the description is just going to be clutter.

> If it is needed, the impl below assumes that the overridden usage starts with
> '$description\n\n`, which may not be true, so why not make it true by only
> allowing to override the `usageWithoutDescription` (with a better name though)
> and `description` separately` and just always add the `\n\n` (or make that
> overridable too?
> 
> In unscripted I have all the usage formatting stuff separated into a
> `UsageFormatter` class which formats `Usage`s which I haven't exposed yet, but
> when I do, the user will be allowed to pass that in to override the
formatting. 
> It provides extension points, instead of an all or nothing overriding.

I'm skeptical of heavily engineering for extensibility before seeing concrete
use cases of the sort of extensibility that's required. A user with simple needs
can already modify the usage additively; a user with entirely idiosyncratic
needs can override [usage] and [usageError] and build them from scratch however
they want. There's a complexity and engineering cost to adding built-in support
for more customization than that, and I don't think it's worth making
theoretical use cases somewhat easier.

https://codereview.chromium.org/797473002/diff/20001/pkg/args/lib/command_run...
pkg/args/lib/command_runner.dart:75: addCommand(new HelpCommand());
On 2014/12/12 17:50:32, Sean Eagan wrote:
> This should only be added when the first sub-command is added.  If there are
> no-subcommands, it isn't needed.

What's the use of a command runner with no commands? I don't think that's a case
we're concerned with.

https://codereview.chromium.org/797473002/diff/20001/pkg/args/lib/command_run...
pkg/args/lib/command_runner.dart:232: ArgResults get globalOptions =>
_globalOptions;
On 2014/12/12 17:50:32, Sean Eagan wrote:
> It's weird to define [options] and [globalOptions] as fields instead of just
> passing them to [run].  I guess that's to avoid having to define them as
> parameters when you don't need them.  That is only a problem because dart
> doesn't allow passing extra arguments.

It's more that it's useful for writing methods without passing extra state
everywhere. I also want to suggest to users that fields are a good place to
store per-call state.

Pub does this a lot; for example, it has an [entrypoint] getter whose value is
based on the options. It's really nice for commands to just be able to say
`entrypoint` wherever and have it work and be cached automatically. Having to
write `getEntrypoint(options)` and manually ensure that it's cached would be
much more burdensome.

In general, I share your discomfort with having fields that change value based
on external actions, but I think in this case—given the single, extremely clear
usage pattern—it's worth it for the usability benefit.

> It also doesn't give you everything you might need if you have 2 or more
> levels of sub-commands.

One can use `parent.options` for that.

> Why not add a `parent` field, and possibly a `root`, `top`, or `global` field
to
> ArgResults and then just pass the leaf ArgResults to [run].
> 
> Also, I don't like calling them `options`, it should be something like
> `argResults` since `ArgResults` already supports things besides options, and
> hopefully will have support for positionals in the future as well.

[argResults] is pretty verbose. It's unfortunate that [options] overlaps with a
more specific term, but I think it'll be more clear in context than something
similarly terse like [results].

https://codereview.chromium.org/797473002/diff/20001/pkg/args/lib/command_run...
pkg/args/lib/command_runner.dart:283: /// Whether or not this command should
appear in help listings.
On 2014/12/12 17:50:32, Sean Eagan wrote:
> appear in -> be hidden from

Done.

https://codereview.chromium.org/797473002/diff/20001/pkg/args/lib/command_run...
pkg/args/lib/command_runner.dart:289: /// commands as long as any of their leaf
commands are visible.
On 2014/12/12 17:50:32, Sean Eagan wrote:
> true -> false
> 
> or "leaf commands are always visible", "branch commands are visible as long
as"

Done.

https://codereview.chromium.org/797473002/diff/20001/pkg/args/lib/command_run...
pkg/args/lib/command_runner.dart:290: bool get hidden {
On 2014/12/12 17:50:32, Sean Eagan wrote:
> Why not add this directly to ArgParser?

See other discussions.

https://codereview.chromium.org/797473002/diff/20001/pkg/args/lib/command_run...
pkg/args/lib/command_runner.dart:314: final aliases = const <String>[];
On 2014/12/12 17:50:32, Sean Eagan wrote:
> How about an AliasCommand for this:
> 
> command.addSubcommand(canonicalCommand);
> command.addSubcommand(new AliasCommand('alias', canonicalCommand));
> 
> Then you could override some details of the aliases by mixing into or
extending
> AliasCommand.  For example, maybe you want to run analytics on which commands
> are being used, including the aliases.

Part of the point of this architecture is to keep all metadata about a command
in one spot, the command class. That includes aliases.

https://codereview.chromium.org/797473002/diff/20001/pkg/args/lib/command_run...
pkg/args/lib/command_runner.dart:343: void printUsage() => print(usage);
On 2014/12/12 17:50:32, Sean Eagan wrote:
> I would think "to control how output is displayed or integrate with a logging
> system" would be best done for all output of a command, not just the help
> output.  There is already `usage` to customize the help content.
> 
> Same for CommandRunner.printUsage.

The usage is the only output that needs to be automatically printed by the
runner.

https://codereview.chromium.org/797473002/diff/20001/pkg/args/lib/command_run...
pkg/args/lib/command_runner.dart:346: void usageError(String message) =>
On 2014/12/12 17:50:32, Sean Eagan wrote:
> usageException?

Done.

Bob Nystrom

LGTM! https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md File pkg/args/README.md (right): https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md#newcode215 pkg/args/README.md:215: // The [name] and [description] fields must be ...

6 years ago (2014-12-16 16:50:42 UTC) #14

LGTM!

https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md
File pkg/args/README.md (right):

https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md#newcode215
pkg/args/README.md:215: // The [name] and [description] fields must be set by
every subclass.
On 2014/12/16 02:07:44, nweiz wrote:
> On 2014/12/12 18:13:22, Bob Nystrom wrote:
> > On 2014/12/11 23:55:24, nweiz wrote:
> > > Changed "set" to "defined".
> > 
> > That still isn't correct. Subclasses don't have to define a field. They have
> to
> > define a getter, and a field is just one way to do that.
> 
> Changed to "property" which matches dartdoc.

Property is perfect, thanks. I just don't want users to be confused about how to
subclass this.

> I'm tired of rehashing this argument over and over. I've stopped complaining
> when you use a property style I don't like; will you do the same for me?

OK, fair enough. :)

https://codereview.chromium.org/797473002/diff/1/pkg/args/README.md#newcode232
pkg/args/README.md:232: command with subcommands can't run its own code, so
[run][] doesn't need to be
On 2014/12/16 02:07:43, nweiz wrote:
> On 2014/12/12 18:13:22, Bob Nystrom wrote:
> > On 2014/12/11 23:55:24, nweiz wrote:
> > > On 2014/12/11 20:25:30, Bob Nystrom wrote:
> > > > Why can't parent commands also be run?
> > > 
> > > Because the pub command runner didn't support it and it wasn't trivial to
> add.
> > > In particular, it's tricky to detect the difference between a runnable
> branch
> > > command and a non-runnable one. I think it's a tractable problem, but not
> one
> > I
> > > wanted to deal with in this CL.
> > 
> > This CL isn't urgent, though, is it?
> 
> Well, it's blocking further work on the development server. But also this is a
> minimal set of functionality for this feature, or at least minimal that
supports
> pub. Even if I were definitely planning to add that functionality, I'd want to
> do it in a separate CL for minimality purposes.
> 
> > I could be wrong but my intuition is that allowing branch commands to be run
> > will simplify the API because it will let you get rid of some duplication
> > between CommandRunner and Command.
> 
> I don't follow your reasoning. The only API difference is that a branch
command
> which implements [run] will have it run; how does that help duplication?

I was thinking it might make it more natural for CommandRunner to subclass
Command but I didn't look into it in detail. I'm fine with doing this in another
patch. I just want to make sure this patch doesn't make that (potential) later
work harder.

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
File pkg/args/lib/src/command_runner.dart (right):

https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
pkg/args/lib/src/command_runner.dart:339: throw new UsageException(message,
_usageWithoutDescription);
On 2014/12/16 02:07:44, nweiz wrote:
> On 2014/12/12 18:13:22, Bob Nystrom wrote:
> > On 2014/12/11 23:55:24, nweiz wrote:
> > > On 2014/12/11 20:25:30, Bob Nystrom wrote:
> > > > These methods are pretty similar to ones in CommandRunner. Maybe make
> > > > CommandRunner a subclass of Command?
> > > 
> > > I think that muddies the semantic distinction too much. The executable is
> not
> > > itself a command.
> > 
> > You think so? It has some extra functionality, but I think almost everything
> > that's true of commands is also true of it:
> > 
> > 1. It can have (sub)commands.
> > 2. It can have options.
> > 3. It has an argParser.
> > 4. It can raise usage errors.
> 
> I don't think behavioral overlap alone is enough to indicate an inheritance
> hierarchy. I also want either a semantic "is-a" relationship that matches the
> hierarchy or a clear use-case for passing the subclass to a function that
> declares the superclass.

I do feel there is an is-a relationship here (I think of a CommandRunner as
being the command that happens to be at the top-level), but I trust your
judgement.

> > I wonder if users will want to be able to have CommandRunners that are also
> used
> > as Commands for some other metacommand. Imagine some package that exposes a
> big
> > executable with a few different commands. But it also defines top-level
> > executables for a few of the more common commands so they can be invoked
> > directly. It would be nice if those could reuse a lot of the same command
> code.
> 
> I really don't think that's very likely. Git, which has more commands and
> subcommands than probably anything, doesn't even do this.
> 
> The UI would also be pretty shaky if we ended up adding support for running
> branch commands. The command runner would incorrectly format errors as though
it
> was the top-level executable.

True. SGTM.

Sean Eagan

On 2014/12/16 00:21:40, nweiz wrote: > The CommandRunner API is much more closely related to ...

6 years ago (2014-12-16 17:40:51 UTC) #15

On 2014/12/16 00:21:40, nweiz wrote:
> The CommandRunner API is much more closely related to the existing args APIs
> that unscripted's. It defines methods through explicit ArgParser calls and
> provides explicit ArgResult objects for handling results. In a sense, it's the
> most obvious means of command dispatching: it just chooses a class and runs a
> method on it.

I agree that a lower-level approach could be useful for folks who want more
control, 
and less reliance on third-parties.  And I get that SDK executables such as pub
fall into this category.

> I think there's definitely room for more experimental APIs like unscripted,
but
> I think it also makes sense to include a straightforward default API in the
main
> args package. Command dispatch is an aspect of arg parsing.

I disagree, if command dispatch were not separate from arg parsing you could add
it directly to [ArgParser].  I've had no problem implementing unscripted without
any command dispatching primitive.  That's why I suggested a separate package,
but it's up to you and Bob (as with everything else here).

> > General thoughts on the API:
> > 
> > We already have `ArgParser` for defining the *interface* for commands and
> > subcommands.  Some stuff in CommandRunner and Command should be added there
> > instead.
> >
> > Unscripted has an internal `Usage` type which defines the CLI interface,
> > wrapping an ArgParser:
> > 
> >   https://github.com/seaneagan/unscripted/blob/master/lib/src/usage.dart
> > 
> > ...adding features, and trying to improve the API, for example:
> > 
> > * Automatically adds help flags and commands.
> > * Adds a description field.
> > * There is a separate `Flag` type which extends `Option`.
> > * `addOption` takes an `Option` or `Flag` instead of several arguments.
> > * Supports individual positional arguments and a rest argument with
> > `addPotional(Positional positional)`.
> > * Supports parsing/validation/transformation of options, flags, and
> positionals,
> > throws appropriate UsageErrors automatically.
> > * Allows commands to be hidden.
> >
> > I would love it if `ArgParser` made some of these changes.
> >
> > (the same file also has an attempt at an improved version of ArgResults, but
> > thats a separate discussion)
> 
> These are mostly out-of-scope for this CL, but feel free to open a separate
> issue against args to discuss them.

About half of them are already in this CL in some form or another, but their
being added to [CommandRunner] and [Command] instead of [ArgParser].

> > But I think Command/CommandRunner should focus on defining the
implementation
> of
> > commands, and dispatching to the appropriate command.
> 
> This is mostly what the commands do, but in order to generate full-fledged
usage
> information it's necessary to include a certain amount of documentary data in
> them. I'm not sure how you're envisioning that moving to ArgParser, especially
> since the user doesn't explicitly interact with it when adding a new command.

As mentioned above, I've already implemented a [Usage] type in unscripted which
has all of these features.  It defines the command-line *interface*.  The
command *implementation* (which gets dispatched to) is handled separately.  It
would make sense to have the same split of interface vs. implementation between
[ArgParser] and [command_runner].

> > I agree with Bob that
> > Command and CommandRunner should share a common base class or one should
> inherit
> > from the other, just like ArgParser defined the interface for the top-level
> > command, and for subcommands.
> 
> I don't like using subclassing, especially public subclassing, just for
sharing
> implementation. A CommandRunner is not a Command, and thus it shouldn't say
that
> it is via the inheritance hierarchy. I also don't think there's any particular
> usage benefit to it doing so.

It is called the "command line", so I think things like "ls", "cd" are
"commands", no?  One example of a usage benefit is overriding [usage].  
You generally want to override usage formatting the same for all commands in the
tree, not individually (as this CL does).  If the usage formatting logic has to
deal with both [CommandRunner]s and [Command]s, that seems pretty tricky.

Also, [CommandRunner] currently has features that would be equally useful to
commands which do not have sub-commands.  So those CLIs are out of luck.

Sean Eagan

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/command_runner.dart File pkg/args/lib/command_runner.dart (right): https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/command_runner.dart#newcode54 pkg/args/lib/command_runner.dart:54: String get _usageWithoutDescription { On 2014/12/16 02:07:44, nweiz wrote: ...

6 years ago (2014-12-16 17:41:05 UTC) #16

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
File pkg/args/lib/command_runner.dart (right):

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:54: String get _usageWithoutDescription {
On 2014/12/16 02:07:44, nweiz wrote:
> A usage error may well come from misunderstanding the usage, but it's
> very unlikely that it will come from having no idea what the command is even
> for, so the description is just going to be clutter.

Makes sense.

> > If it is needed, the impl below assumes that the overridden usage starts
with
> > '$description\n\n`, which may not be true, so why not make it true by only
> > allowing to override the `usageWithoutDescription` (with a better name
though)
> > and `description` separately` and just always add the `\n\n` (or make that
> > overridable too?
> > 
> > In unscripted I have all the usage formatting stuff separated into a
> > `UsageFormatter` class which formats `Usage`s which I haven't exposed yet,
but
> > when I do, the user will be allowed to pass that in to override the
> formatting. 
> > It provides extension points, instead of an all or nothing overriding.
> 
> I'm skeptical of heavily engineering for extensibility before seeing concrete
> use cases of the sort of extensibility that's required. A user with simple
needs
> can already modify the usage additively; a user with entirely idiosyncratic
> needs can override [usage] and [usageError] and build them from scratch
however
> they want. There's a complexity and engineering cost to adding built-in
support
> for more customization than that, and I don't think it's worth making
> theoretical use cases somewhat easier.

There already is customization of [usage] via [description], [invocation],
[usageFooter], which cease to be meaningful once [usage] is overridden.  That's
why it makes sense to move them all to a separate type e.g. [UsageFormatter]
which can be used by the default implementation of [usage], and if folks want,
by their overrides as well, but they don't have to.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:75: addCommand(new HelpCommand());
On 2014/12/16 02:07:44, nweiz wrote:
> On 2014/12/12 17:50:32, Sean Eagan wrote:
> > This should only be added when the first sub-command is added.  If there are
> > no-subcommands, it isn't needed.
> 
> What's the use of a command runner with no commands? I don't think that's a
case
> we're concerned with.

The problem is, [CommandRunner] as currently defined contains features that
would be just as useful to commands which do not have sub-commands, i.e.
automatically adding help flags, defining how to display help text, displaying
help text when that flag is invoked, and a small start at defining positional
arguments.  Yet, [CommandRunner] doesn't allow commands without subcommands. 
The two ways to solve this are:

1) Allow [CommandRunner] to work without sub-commands (probably involves
de-duplication of [CommandRunner] and [Command].)
2) Move the extra features to [ArgParser].

I much prefer 2).

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:232: ArgResults get globalOptions =>
_globalOptions;
On 2014/12/16 02:07:44, nweiz wrote:
> On 2014/12/12 17:50:32, Sean Eagan wrote:
> > It's weird to define [options] and [globalOptions] as fields instead of just
> > passing them to [run].  I guess that's to avoid having to define them as
> > parameters when you don't need them.  That is only a problem because dart
> > doesn't allow passing extra arguments.
> 
> It's more that it's useful for writing methods without passing extra state
> everywhere. I also want to suggest to users that fields are a good place to
> store per-call state.
> 
> Pub does this a lot; for example, it has an [entrypoint] getter whose value is
> based on the options. It's really nice for commands to just be able to say
> `entrypoint` wherever and have it work and be cached automatically. Having to
> write `getEntrypoint(options)` and manually ensure that it's cached would be
> much more burdensome.

Once the command implementation gets sufficiently complex to need to be broken
into multiple methods that access the same state (e.g. entryPoint), I think a
better pattern is to delegate to a separate domain class with those methods than
add those methods to the [Command] / [CommandRunner].  The [run] impl would just
extract info from the [ArgResults] it receives, and pass those to the domain
class's constructor.  In unscripted, this is mostly automatically done for the
user (they can optionally provide metadata to map command-line Strings to domain
types, which are then injected into the domain class or function).

> > It also doesn't give you everything you might need if you have 2 or more
> > levels of sub-commands.
> 
> One can use `parent.options` for that.

IMO options.parent would be more obvious, and it would also be useful for people
using [ArgResults] outside of [command_runner], for example I've wanted this
when implementing unscripted.

> > Also, I don't like calling them `options`, it should be something like
> > `argResults` since `ArgResults` already supports things besides options, and
> > hopefully will have support for positionals in the future as well.
> 
> [argResults] is pretty verbose.

I think the naming accuracy and clarity gained from [argResults] over [options]
is well worth the extra 3 characters.

> It's unfortunate that [options] overlaps with a
> more specific term

It's not unfortunate yet, you can still fix it :P.  The name would be mostly
irrelevant if it were just a parameter to [run] instead of a field.  Also,
[ArgResults] already has a field named [options], so that leads to
[options.options] which is super unreadable.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:314: final aliases = const <String>[];
On 2014/12/16 02:07:44, nweiz wrote:
> On 2014/12/12 17:50:32, Sean Eagan wrote:
> > How about an AliasCommand for this:
> > 
> > command.addSubcommand(canonicalCommand);
> > command.addSubcommand(new AliasCommand('alias', canonicalCommand));
> > 
> > Then you could override some details of the aliases by mixing into or
> extending
> > AliasCommand.  For example, maybe you want to run analytics on which
commands
> > are being used, including the aliases.
> 
> Part of the point of this architecture is to keep all metadata about a command
> in one spot, the command class. That includes aliases.

Agreed if there is some use of that metadata, and in the case of aliases it
could definitely be used to improve the help text by noting that they are alias
commands for the canonical command.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:343: void printUsage() => print(usage);
On 2014/12/16 02:07:44, nweiz wrote:
> The usage is the only output that needs to be automatically printed by the
> runner.

Unscripted also automatically outputs usage errors, and its [UsageFormatter] has
a [formatUsageError] to hook into the content (once [UsageFormatter] is
exposed).  [command_runner] requires boilerplate for that, which just means
people will need to build higher-level abstractions on top of it, which is fine.

I plan to add a Logger plugin to unscripted where verbosity argument(s) are
added (e.g. --verbose, --verbosity=...) and the Logger's verbosity is set based
on that, usage errors are automatically logged with a certain verbosity, and you
can get more verbose help output via `--verbose --help`.

https://chromiumcodereview.appspot.com/797473002/diff/40001/pkg/args/lib/comm...
File pkg/args/lib/command_runner.dart (right):

https://chromiumcodereview.appspot.com/797473002/diff/40001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:89: throw new UsageException(message,
_usageWithoutDescription);
This needs to hook into [usage], or you need to expose
[usageWithoutDescription], otherwise when [usage] is overridden, it doesn't take
effect here.

Sean Eagan

On 2014/12/16 02:07:44, nweiz wrote: > https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_runner.dart#newcode70 > pkg/args/lib/src/command_runner.dart:70: CommandRunner(this.executableName, > this.description) { > On ...

6 years ago (2014-12-16 18:37:24 UTC) #17

On 2014/12/16 02:07:44, nweiz wrote:
>
https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
> pkg/args/lib/src/command_runner.dart:70: CommandRunner(this.executableName,
> this.description) {
> On 2014/12/12 17:50:31, Sean Eagan wrote:
> > The executable name should rarely if ever need to be explicitly set, as it
can
> > be deduced from dart:io's `Platform.script`.  So this parameter should be
> > optional if it's included.
> 
> args doesn't currently use dart:io. Adding an import now would be a breaking
> change, and I don't think it's worth it just to make the executable name
> optional.

The import would be in the `command_runner.dart` lib (or possibly
(`usage_formatter.dart` or similar), not `args.dart`, so not a breaking change,
and there's not many cases for using `args` from the browser anyway.

> I'm also wary of depending too heavily on Platform.script.

I wasn't aware of any issues with it, would be curious to know what those are
(as I'm using it from unscripted).

> > Description should be optional too, if nothing else so that `description: `
> > shows up at the call site to make it clear what the String is for.
> 
> What would we do for the usage information if it's not provided?

The usage just wouldn't have a description in it, I believe there are several
standard unix commands which don't include one for example.  It might be useful
for `tool` (as opposed to `bin`) scripts for example, to not have to include a
description.

>
https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
> pkg/args/lib/src/command_runner.dart:112: } on FormatException catch (error) {
> On 2014/12/12 17:50:31, Sean Eagan wrote:
> > Why not just fix ArgParser.parse to throw UsageError instead of
> FormatException?
> 
> I'm not a fan of that for a few reasons:
> 
> * It would be a backwards-incompatible change unless we made UsageError extend
> FormatException, which I don't think makes sense.
> * I think FormatException still makes sense for parsing args. It's consistent
> with other parse functions' exceptions.

As a consumer of `args`, I don't think it's very helpful, whereas UsageException
is quite helpful.

> * Even if we made that change, we'd still have to catch and rethrow here
because
> the arg parser doesn't know the runner's full usage string.

As mentioned elsewhere, I think the usage formatting should be configured
globally for the entire command tree, and so the UsageError should just contain
a reference to the [ArgParser], and then that should bubble up to a usage
formatter.

Also, you can't be sure a FormatException is from invalid args here, it could
just be from a random `int.parse`, or some callback the user has provided (or
will provide in the future).

>
https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
> pkg/args/lib/src/command_runner.dart:299: final takesArguments = true;
> On 2014/12/12 17:50:31, Sean Eagan wrote:
> > This is too coarse grained.  Unscripted allows adding individual positional
> > arguments, and also a trailing "rest" argument, with the same sort of
metadata
> > as options and flags.  It can then validate the right number and type of
> > positional args as well, and make those easily accessible.
> 
> That would be a much broader change largely in ArgParser, out of scope of this
> CL. If we did add it we could revise the use of this field

If positional args are added, then this field would only make sense as being
derived from whether any positional args have been added, it would belong on
[ArgParser].  Adding `takesArguments` in this CL makes it impossible to add
positional args in the future without a breaking change AFAICT.

, but I'll tell you
> now that I'm not a big fan of that approach; positional arguments come in too
> many different heterogeneous forms to effectively capture in an API.

I thought you'd be on my side here :), see:

https://dartbug.com/20042

So long as you don't implement something akin to type-based overloading, it's
pretty straight-forward.

> > The default should be no positional args allowed.  The cli author should
> > explicitly define their positionals just as they do with named arguments
> > (options and flags).
> 
> I think it's most important that this match the behavior of ArgParser. In
> general I think most commands also do want positional arguments. For example,
> about 2/3s of pub's commands take positional arguments.

I would be a pretty big change, not sure what type of changes y'all are willing
to make.

>
https://codereview.chromium.org/797473002/diff/1/pkg/args/lib/src/command_run...
> pkg/args/lib/src/command_runner.dart:307: final aliases = const <String>[];
> On 2014/12/12 17:50:31, Sean Eagan wrote:
> > If adding this, it ought to be exposed on ArgParser directly, it's just as
> > meaningful there.
> 
> I'm open to adding this to ArgParser.addCommand, but not in this CL. The API
> here is the same either way, since the user isn't calling that method
directly.
> Converting ArgParser.addCommand to take named arguments would also be a
breaking
> change.

That's why the style guide should read "DONT Use optional positionals on public
APIs" :).

Sean Eagan

There's probably no chance of pub using unscripted, but just to demo some of the ...

6 years ago (2014-12-16 21:27:17 UTC) #18

nweiz

> I disagree, if command dispatch were not separate from arg parsing you could > ...

6 years ago (2014-12-17 00:04:59 UTC) #19

> I disagree, if command dispatch were not separate from arg parsing you could
> add it directly to [ArgParser]. I've had no problem implementing unscripted
> without any command dispatching primitive. That's why I suggested a separate
> package, but it's up to you and Bob (as with everything else here).

Sorry, I should have said "an aspect of arg handling". Regardless, I think it's
a useful thing to include.

> About half of them are already in this CL in some form or another, but their
> being added to [CommandRunner] and [Command] instead of [ArgParser].

Not necessarily instead of. I didn't want to do any major reworking of ArgParser
for this, but I did want various features in the command runner, so I added them
there. Your suggestions for ArgParser are orthogonal; even if hidden commands
*were* added to ArgParser, we'd still want users to use [Command.hidden] when
using [CommandRunner]s.

> As mentioned above, I've already implemented a [Usage] type in unscripted
> which has all of these features. It defines the command-line *interface*. The
> command *implementation* (which gets dispatched to) is handled separately. It
> would make sense to have the same split of interface vs. implementation
> between [ArgParser] and [command_runner].

I'm not deeply familiar with unscripted, and I'm not eager to delve through its
implementation to learn about its Usage class.

> It is called the "command line", so I think things like "ls", "cd" are
> "commands", no? One example of a usage benefit is overriding [usage]. You
> generally want to override usage formatting the same for all commands in the
> tree, not individually (as this CL does). If the usage formatting logic has to
> deal with both [CommandRunner]s and [Command]s, that seems pretty tricky.

As I mentioned to Bob, I don't think sharing behavior is sufficient
justification for creating an inheritance hierarchy.

> Also, [CommandRunner] currently has features that would be equally useful to
> commands which do not have sub-commands. So those CLIs are out of luck.

I'm not sure which features you're referring to. The only APIs it exposes for
use during invocation are pure metadata, which is accessible through
[Command.runner].

> There's probably no chance of pub using unscripted, but just to demo some of
> the stuff I'm advocating for here, I went ahead and mocked out the interface
> for a couple of pub commands using unscripted, if you're curious what that
> would look like:
>
> https://github.com/seaneagan/pub_unscripted_example

Personally, I find that much harder to read and reason about than the current
design or the new CommandRunner-based design.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
File pkg/args/lib/command_runner.dart (right):

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:54: String get _usageWithoutDescription {
On 2014/12/16 17:41:05, Sean Eagan wrote:
> On 2014/12/16 02:07:44, nweiz wrote:
> > A usage error may well come from misunderstanding the usage, but it's
> > very unlikely that it will come from having no idea what the command is even
> > for, so the description is just going to be clutter.
> 
> Makes sense.
> 
> > > If it is needed, the impl below assumes that the overridden usage starts
> with
> > > '$description\n\n`, which may not be true, so why not make it true by only
> > > allowing to override the `usageWithoutDescription` (with a better name
> though)
> > > and `description` separately` and just always add the `\n\n` (or make that
> > > overridable too?
> > > 
> > > In unscripted I have all the usage formatting stuff separated into a
> > > `UsageFormatter` class which formats `Usage`s which I haven't exposed yet,
> but
> > > when I do, the user will be allowed to pass that in to override the
> > formatting. 
> > > It provides extension points, instead of an all or nothing overriding.
> > 
> > I'm skeptical of heavily engineering for extensibility before seeing
concrete
> > use cases of the sort of extensibility that's required. A user with simple
> needs
> > can already modify the usage additively; a user with entirely idiosyncratic
> > needs can override [usage] and [usageError] and build them from scratch
> however
> > they want. There's a complexity and engineering cost to adding built-in
> support
> > for more customization than that, and I don't think it's worth making
> > theoretical use cases somewhat easier.
> 
> There already is customization of [usage] via [description], [invocation],
> [usageFooter], which cease to be meaningful once [usage] is overridden. 
That's
> why it makes sense to move them all to a separate type e.g. [UsageFormatter]
> which can be used by the default implementation of [usage], and if folks want,
> by their overrides as well, but they don't have to.

But the vastly-more-common use case becomes more difficult in that model, in
service of supporting a theoretical one. This is a convenience API, and as such
it will generally trade power for making the blessed path more straightforward.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:75: addCommand(new HelpCommand());
On 2014/12/16 17:41:05, Sean Eagan wrote:
> On 2014/12/16 02:07:44, nweiz wrote:
> > On 2014/12/12 17:50:32, Sean Eagan wrote:
> > > This should only be added when the first sub-command is added.  If there
are
> > > no-subcommands, it isn't needed.
> > 
> > What's the use of a command runner with no commands? I don't think that's a
> case
> > we're concerned with.
> 
> The problem is, [CommandRunner] as currently defined contains features that
> would be just as useful to commands which do not have sub-commands, i.e.
> automatically adding help flags, defining how to display help text, displaying
> help text when that flag is invoked, and a small start at defining positional
> arguments.  Yet, [CommandRunner] doesn't allow commands without subcommands. 
> The two ways to solve this are:
> 
> 1) Allow [CommandRunner] to work without sub-commands (probably involves
> de-duplication of [CommandRunner] and [Command].)
> 2) Move the extra features to [ArgParser].
> 
> I much prefer 2).

I'm skeptical that integrating with a separate library is in fact easier than
manually implementing a help flag manually, but that's a separate discussion. We
both agree that we'd prefer a different solution than using CommandRunner to get
`--help` support for non-command-based applications, so it seems clear that
supporting that use-case isn't particularly beneficial.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:232: ArgResults get globalOptions =>
_globalOptions;
On 2014/12/16 17:41:05, Sean Eagan wrote:
> On 2014/12/16 02:07:44, nweiz wrote:
> > On 2014/12/12 17:50:32, Sean Eagan wrote:
> > > It's weird to define [options] and [globalOptions] as fields instead of
just
> > > passing them to [run].  I guess that's to avoid having to define them as
> > > parameters when you don't need them.  That is only a problem because dart
> > > doesn't allow passing extra arguments.
> > 
> > It's more that it's useful for writing methods without passing extra state
> > everywhere. I also want to suggest to users that fields are a good place to
> > store per-call state.
> > 
> > Pub does this a lot; for example, it has an [entrypoint] getter whose value
is
> > based on the options. It's really nice for commands to just be able to say
> > `entrypoint` wherever and have it work and be cached automatically. Having
to
> > write `getEntrypoint(options)` and manually ensure that it's cached would be
> > much more burdensome.
> 
> Once the command implementation gets sufficiently complex to need to be broken
> into multiple methods that access the same state (e.g. entryPoint), I think a
> better pattern is to delegate to a separate domain class with those methods
than
> add those methods to the [Command] / [CommandRunner].  The [run] impl would
just
> extract info from the [ArgResults] it receives, and pass those to the domain
> class's constructor.  In unscripted, this is mostly automatically done for the
> user (they can optionally provide metadata to map command-line Strings to
domain
> types, which are then injected into the domain class or function).

You're describing a pretty complex architecture there. It may be that it's
better for large-scale applications, but I don't want to mandate it for users.
For a mid-sized app like pub, hanging getters off the command class works very
well.

> > > It also doesn't give you everything you might need if you have 2 or more
> > > levels of sub-commands.
> > 
> > One can use `parent.options` for that.
> 
> IMO options.parent would be more obvious, and it would also be useful for
people
> using [ArgResults] outside of [command_runner], for example I've wanted this
> when implementing unscripted.

I'm not opposed to adding [ArgResults.parent] in another CL, but I think we'd
still keep the [parent] getter here so that instance variables on parent classes
can be accessed.

> > > Also, I don't like calling them `options`, it should be something like
> > > `argResults` since `ArgResults` already supports things besides options,
and
> > > hopefully will have support for positionals in the future as well.
> > 
> > [argResults] is pretty verbose.
> 
> I think the naming accuracy and clarity gained from [argResults] over
[options]
> is well worth the extra 3 characters.
> 
> > It's unfortunate that [options] overlaps with a
> > more specific term
> 
> It's not unfortunate yet, you can still fix it :P.  The name would be mostly
> irrelevant if it were just a parameter to [run] instead of a field.  Also,
> [ArgResults] already has a field named [options], so that leads to
> [options.options] which is super unreadable.

All right, I'm convinced. Changed to [argResults].

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:314: final aliases = const <String>[];
On 2014/12/16 17:41:05, Sean Eagan wrote:
> On 2014/12/16 02:07:44, nweiz wrote:
> > On 2014/12/12 17:50:32, Sean Eagan wrote:
> > > How about an AliasCommand for this:
> > > 
> > > command.addSubcommand(canonicalCommand);
> > > command.addSubcommand(new AliasCommand('alias', canonicalCommand));
> > > 
> > > Then you could override some details of the aliases by mixing into or
> > extending
> > > AliasCommand.  For example, maybe you want to run analytics on which
> commands
> > > are being used, including the aliases.
> > 
> > Part of the point of this architecture is to keep all metadata about a
command
> > in one spot, the command class. That includes aliases.
> 
> Agreed if there is some use of that metadata, and in the case of aliases it
> could definitely be used to improve the help text by noting that they are
alias
> commands for the canonical command.

The fact that the metadata is used by the dispatcher as opposed to the command
isn't really relevant to most users. From their perspective, the aliases (like
the name) is a property of the command.

At least for pub's use-cases, we don't want any of the aliases to be visible. I
also don't know that we have a clear and consistent place we'd list aliases; if
users want to do so in their preferred format, they can add them to the
description.

https://chromiumcodereview.appspot.com/797473002/diff/20001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:343: void printUsage() => print(usage);
On 2014/12/16 17:41:05, Sean Eagan wrote:
> On 2014/12/16 02:07:44, nweiz wrote:
> > The usage is the only output that needs to be automatically printed by the
> > runner.
> 
> Unscripted also automatically outputs usage errors, and its [UsageFormatter]
has
> a [formatUsageError] to hook into the content (once [UsageFormatter] is
> exposed).  [command_runner] requires boilerplate for that, which just means
> people will need to build higher-level abstractions on top of it, which is
fine.
> 
> I plan to add a Logger plugin to unscripted where verbosity argument(s) are
> added (e.g. --verbose, --verbosity=...) and the Logger's verbosity is set
based
> on that, usage errors are automatically logged with a certain verbosity, and
you
> can get more verbose help output via `--verbose --help`.

This is another situation where I'm not sure the benefit of having a library do
something automatically is worth the complexity of a system that enables the
library to do so.

https://chromiumcodereview.appspot.com/797473002/diff/40001/pkg/args/lib/comm...
File pkg/args/lib/command_runner.dart (right):

https://chromiumcodereview.appspot.com/797473002/diff/40001/pkg/args/lib/comm...
pkg/args/lib/command_runner.dart:89: throw new UsageException(message,
_usageWithoutDescription);
On 2014/12/16 17:41:05, Sean Eagan wrote:
> This needs to hook into [usage], or you need to expose
> [usageWithoutDescription], otherwise when [usage] is overridden, it doesn't
take
> effect here.

It used to hook into [usage], but based on Bob's feedback I undid that. I think
if a user wants to modify [usage] in a completely idiosyncratic way, we should
expect they'll also want a completely idiosyncratic format for whatever usage
they append to the exception. Automatically basing it on [usage] in any way
makes assumptions about their formatting that are likely to be false.

nweiz

6 years ago (2014-12-17 00:06:20 UTC) #20

Message was sent while issue was closed.

Committed patchset #3 (id:40001) manually as 42418 (presubmit successful).

Issue 797473002: Add a CommandRunner class for dispatching commands to args. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Code review changes #

Patch Set 3 : Code review changes #

Messages