Add a CommandRunner class for dispatching commands to args.

pkg/args/lib/command_runner.dart

+// Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+library args.command_runner;
+
+import 'dart:async';
+import 'dart:collection';
+import 'dart:math' as math;
+
+import 'src/arg_parser.dart';
+import 'src/arg_results.dart';
+import 'src/help_command.dart';
+import 'src/usage_exception.dart';
+import 'src/utils.dart';
+
+export 'src/usage_exception.dart';
+
+/// A class for invoking [Commands] based on raw command-line arguments.
+class CommandRunner {
+  /// The name of the executable being run.
+  ///
+  /// Used for error reporting.

[Sean Eagan, 2014/12/12 17:50:32]

also used for usage text.

[nweiz, 2014/12/16 02:07:44]

Done.

+  final String executableName;
+
+  /// A short description of this executable.
+  final String description;
+
+  /// A single-line template for how to invoke this executable.
+  ///
+  /// Defaults to "$executableName [command] <arguments>". Subclasses can
+  /// override this for a more specific template.
+  String get usageTemplate => "$executableName [command] <arguments>";

[Sean Eagan, 2014/12/12 17:50:32]

invocationTemplate?
invocationHelp?

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

[nweiz, 2014/12/16 02:07:44]

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".

+
+  /// Generates a string displaying usage information for the executable.
+  ///
+  /// This includes usage for the global arguments as well as a list of
+  /// top-level commands.
+  String get usage {
+    return '''
+$description
+
+Usage: $usageTemplate
+
+Global options:
+${argParser.usage}
+
+${_getCommandUsage(_commands)}
+
+Run "$executableName help [command]" for more information about a command.''';
+  }
+
+  /// Returns [usage] with [description] removed from the beginning.
+  String get _usageWithoutDescription {

[Sean Eagan, 2014/12/12 17:50:32]

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.

[nweiz, 2014/12/16 02:07:44]

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.
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.

[Sean Eagan, 2014/12/16 17:41:05]

Makes sense.
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.

[nweiz, 2014/12/17 00:04:59]

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.

+    // Base this on the return value of [usage] so that subclasses can override
+    // usage and have their changes reflected here.
+    return usage.replaceFirst("$description\n\n", "");
+  }
+
+  /// An unmodifiable view of all top-level commands defined for this runner.
+  Map<String, Command> get commands =>
+      new UnmodifiableMapView(_commands);
+  final _commands = new Map<String, Command>();
+
+  /// The top-level argument parser.
+  ///
+  /// Global options should be registered with this parser; they'll end up
+  /// available via [Command.globalOptions]. Commands should be registered with
+  /// [addCommand] rather than directly on the parser.
+  final argParser = new ArgParser();
+
+  CommandRunner(this.executableName, this.description) {

[Sean Eagan, 2014/12/12 17:50:32]

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.

+    argParser.addFlag('help', abbr: 'h', negatable: false,
+        help: 'Print this usage information.');
+    addCommand(new HelpCommand());

[Sean Eagan, 2014/12/12 17:50:32]

This should only be added when the first sub-command is added.  If there are
no-subcommands, it isn't needed.

[nweiz, 2014/12/16 02:07:44]

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

[Sean Eagan, 2014/12/16 17:41:05]

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).

[nweiz, 2014/12/17 00:04:59]

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.

+  }
+
+  /// Prints the usage information for this runner.
+  ///
+  /// This is called internally by [run] and can be overridden by subclasses to
+  /// control how output is displayed or integrate with a logging system.
+  void printUsage() => print(usage);
+
+  /// Throws a [UsageException] with [message].
+  void usageError(String message) =>
+      throw new UsageException(message, _usageWithoutDescription);
+
+  /// Adds [Command] as a top-level command to this runner.
+  void addCommand(Command command) {
+    var names = [command.name]..addAll(command.aliases);
+    for (var name in names) {
+      _commands[name] = command;
+      argParser.addCommand(name, command.argParser);
+    }
+    command._runner = this;
+  }
+
+  /// Parses [args] and invokes [Command.run] on the chosen command.
+  ///
+  /// This always returns a [Future] in case the command is asynchronous. The
+  /// [Future] will throw a [UsageError] if [args] was invalid.
+  Future run(Iterable<String> args) =>
+      new Future.sync(() => runCommand(parse(args)));
+
+  /// Parses [args] and returns the result, converting a [FormatException] to a
+  /// [UsageException].
+  ///
+  /// This is notionally a protected method. It may be overridden or called from
+  /// subclasses, but it shouldn't be called externally.
+  ArgResults parse(Iterable<String> args) {
+    try {
+      // TODO(nweiz): if arg parsing fails for a command, print that command's
+      // usage, not the global usage.
+      return argParser.parse(args);
+    } on FormatException catch (error) {
+      usageError(error.message);
+    }
+  }
+
+  /// Runs the command specified by [topLevelOptions].
+  ///
+  /// This is notionally a protected method. It may be overridden or called from
+  /// subclasses, but it shouldn't be called externally.
+  ///
+  /// It's useful to override this to handle global flags and/or wrap the entire
+  /// command in a block. For example, you might handle the `--verbose` flag
+  /// here to enable verbose logging before running the command.
+  Future runCommand(ArgResults topLevelOptions) {
+    return new Future.sync(() {
+      var options = topLevelOptions;
+      var commands = _commands;
+      var command;
+      var commandString = executableName;
+
+      while (commands.isNotEmpty) {
+        if (options.command == null) {
+          if (options.rest.isEmpty) {
+            if (command == null) {
+              // No top-level command was chosen.
+              printUsage();
+              return new Future.value();
+            }
+
+            command.usageError('Missing subcommand for "$commandString".');
+          } else {
+            if (command == null) {
+              usageError(
+                  'Could not find a command named "${options.rest[0]}".');
+            }
+
+            command.usageError('Could not find a subcommand named '
+                '"${options.rest[0]}" for "$commandString".');
+          }
+        }
+
+        // Step into the command.
+        var parent = command;
+        options = options.command;
+        command = commands[options.name];
+        command._globalOptions = topLevelOptions;
+        command._options = options;
+        commands = command._subcommands;
+        commandString += " ${options.name}";
+
+        if (options['help']) {
+          command.printUsage();
+          return new Future.value();
+        }
+      }
+
+      // Make sure there aren't unexpected arguments.
+      if (!command.takesArguments && options.rest.isNotEmpty) {
+        command.usageError(
+            'Command "${options.name}" does not take any arguments.');
+      }
+
+      return command.run();
+    });
+  }
+}
+
+/// A single command.
+///
+/// A command is known as a "leaf command" if it has no subcommands and is meant
+/// to be run. Leaf commands must override [run].
+///
+/// A command with subcommands is known as a "branch command" and cannot be run
+/// itself. It should call [addSubcommand] (often from the constructor) to
+/// register subcommands.
+abstract class Command {
+  /// The name of this command.
+  String get name;
+
+  /// A short description of this command.
+  String get description;
+
+  /// A single-line template for how to invoke this command (e.g. `"pub get
+  /// [package]"`).
+  String get usageTemplate {
+    var parents = [name];
+    for (var command = parent; command != null; command = command.parent) {
+      parents.add(command.name);
+    }
+    parents.add(runner.executableName);
+
+    var invocation = parents.reversed.join(" ");
+    return _subcommands.isNotEmpty ?
+        "$invocation [subcommand] <arguments>" :
+        "$invocation <arguments>";
+  }
+
+  /// The command's parent command, if this is a subcommand.
+  ///
+  /// This will be `null` until [Command.addSubcommmand] has been called with
+  /// this command.
+  Command get parent => _parent;
+  Command _parent;
+
+  /// The command runner for this command.
+  ///
+  /// This will be `null` until [CommandRunner.addCommand] has been called with
+  /// this command or one of its parents.
+  CommandRunner get runner {
+    if (parent == null) return _runner;
+    return parent.runner;
+  }
+  CommandRunner _runner;
+
+  /// The parsed global options.
+  ///
+  /// This will be `null` until just before [Command.run] is called.
+  ArgResults get globalOptions => _globalOptions;

[Sean Eagan, 2014/12/12 17:50:32]

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.

[nweiz, 2014/12/16 02:07:44]

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.
One can use `parent.options` for that.
[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].

[Sean Eagan, 2014/12/16 17:41:05]

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).
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 think the naming accuracy and clarity gained from [argResults] over [options]
is well worth the extra 3 characters.
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.

[nweiz, 2014/12/17 00:04:59]

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.
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.
All right, I'm convinced. Changed to [argResults].

+  ArgResults _globalOptions;
+
+  /// The parsed options for this command.
+  ///
+  /// This will be `null` until just before [Command.run] is called.
+  ArgResults get options => _options;
+  ArgResults _options;
+
+  /// The argument parser for this command.
+  ///
+  /// Options for this command should be registered with this parser (often in
+  /// the constructor); they'll end up available via [options]. Subcommands
+  /// should be registered with [addSubcommand] rather than directly on the
+  /// parser.
+  final argParser = new ArgParser();
+
+  /// Generates a string displaying usage information for this command.
+  ///
+  /// This includes usage for the command's arguments as well as a list of
+  /// subcommands, if there are any.
+  String get usage {
+    var buffer = new StringBuffer()
+        ..writeln(description)
+        ..writeln()
+        ..writeln('Usage: $usageTemplate')
+        ..writeln(argParser.usage);
+
+    if (_subcommands.isNotEmpty) {
+      buffer.writeln();
+      buffer.writeln(_getCommandUsage(_subcommands, isSubcommand: true));
+    }
+
+    buffer.writeln();
+    buffer.write('Run "${runner.executableName} help" to see global options.');
+
+    return buffer.toString();
+  }
+
+  /// Returns [usage] with [description] removed from the beginning.
+  String get _usageWithoutDescription {
+    // Base this on the return value of [usage] so that subclasses can override
+    // usage and have their changes reflected here.
+    return usage.replaceFirst("$description\n\n", "");
+  }
+
+  /// An unmodifiable view of all sublevel commands of this command.
+  Map<String, Command> get subcommands =>
+      new UnmodifiableMapView(_subcommands);
+  final _subcommands = new Map<String, Command>();
+
+  /// Whether or not this command should appear in help listings.

[Sean Eagan, 2014/12/12 17:50:32]

appear in -> be hidden from

[nweiz, 2014/12/16 02:07:44]

Done.

+  ///
+  /// This is intended to be overridden by commands that want to mark themselves
+  /// hidden.
+  ///
+  /// By default, this is always true for leaf commands. It's true for branch
+  /// commands as long as any of their leaf commands are visible.

[Sean Eagan, 2014/12/12 17:50:32]

true -> false

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

[nweiz, 2014/12/16 02:07:44]

Done.

+  bool get hidden {

[Sean Eagan, 2014/12/12 17:50:32]

Why not add this directly to ArgParser?

[nweiz, 2014/12/16 02:07:44]

See other discussions.

+    // Leaf commands are visible by default.
+    if (_subcommands.isEmpty) return false;
+
+    // Otherwise, a command is hidden if all of its subcommands are.
+    return _subcommands.values.every((subcommand) => subcommand.hidden);
+  }
+
+  /// Whether or not this command takes positional arguments in addition to
+  /// options.
+  ///
+  /// If false, [CommandRunner.run] will throw a [UsageException] if arguments
+  /// are provided. Defaults to true.
+  ///
+  /// This is intended to be overridden by commands that don't want to receive
+  /// arguments. It has no effect for branch commands.
+  final takesArguments = true;

[Sean Eagan, 2014/12/12 17:50:32]

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.

+
+  /// Alternate names for this command.
+  ///
+  /// These names won't be used in the documentation, but they will work when
+  /// invoked on the command line.
+  ///
+  /// This is intended to be overridden.
+  final aliases = const <String>[];

[Sean Eagan, 2014/12/12 17:50:32]

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.

[nweiz, 2014/12/16 02:07:44]

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

[Sean Eagan, 2014/12/16 17:41:05]

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.

[nweiz, 2014/12/17 00:04:59]

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.

+
+  Command() {
+    argParser.addFlag('help', abbr: 'h', negatable: false,
+        help: 'Print this usage information.');
+  }
+
+  /// Runs this command.
+  ///
+  /// If this returns a [Future], [CommandRunner.run] won't complete until the
+  /// returned [Future] does. Otherwise, the return value is ignored.
+  run() {
+    throw new UnimplementedError("Leaf command $this must implement run().");
+  }
+
+  /// Adds [Command] as a subcommand of this.
+  void addSubcommand(Command command) {
+    var names = [command.name]..addAll(command.aliases);
+    for (var name in names) {
+      _subcommands[name] = command;
+      argParser.addCommand(name, command.argParser);
+    }
+    command._parent = this;
+  }
+
+  /// Prints the usage information for this command.
+  ///
+  /// This is called internally by [run] and can be overridden by subclasses to
+  /// control how output is displayed or integrate with a logging system.
+  void printUsage() => print(usage);

[Sean Eagan, 2014/12/12 17:50:32]

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.

[nweiz, 2014/12/16 02:07:44]

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

[Sean Eagan, 2014/12/16 17:41:05]

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`.

[nweiz, 2014/12/17 00:04:59]

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.

+
+  /// Throws a [UsageException] with [message].
+  void usageError(String message) =>

[Sean Eagan, 2014/12/12 17:50:32]

usageException?

[nweiz, 2014/12/16 02:07:44]

Done.

+      throw new UsageException(message, _usageWithoutDescription);
+}
+
+/// Returns a string representation of [commands] fit for use in a usage string.
+///
+/// [isSubcommand] indicates whether the commands should be called "commands" or
+/// "subcommands".
+String _getCommandUsage(Map<String, Command> commands,
+    {bool isSubcommand: false}) {
+  // Don't include aliases.
+  var names = commands.keys
+      .where((name) => !commands[name].aliases.contains(name));
+
+  // Filter out hidden ones, unless they are all hidden.
+  var visible = names.where((name) => !commands[name].hidden);
+  if (visible.isNotEmpty) names = visible;
+
+  // Show the commands alphabetically.
+  names = names.toList()..sort();
+  var length = names.map((name) => name.length).reduce(math.max);
+
+  var buffer = new StringBuffer(
+      'Available ${isSubcommand ? "sub" : ""}commands:');
+  for (var name in names) {
+    buffer.writeln();
+    buffer.write('  ${padRight(name, length)}   '
+        '${commands[name].description.split("\n").first}');
+  }
+
+  return buffer.toString();
+}