Add a CommandRunner class for dispatching commands to args.

pkg/args/lib/src/command_runner.dart

Index: pkg/args/lib/src/command_runner.dart
diff --git a/pkg/args/lib/src/command_runner.dart b/pkg/args/lib/src/command_runner.dart
new file mode 100644
index 0000000000000000000000000000000000000000..cfce5e8b124314690fe2c2d65d79b052a26ed259
--- /dev/null
+++ b/pkg/args/lib/src/command_runner.dart
@@ -0,0 +1,369 @@
+// 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 'arg_parser.dart';
+import 'arg_results.dart';
+import 'help_command.dart';
+import 'usage_exception.dart';
+import 'utils.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.
+  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>";
+
+  /// 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:
+${topLevelArgParser.usage}
+
+${_getCommandUsage(_topLevelCommands)}
+
+Run "$executableName help [command]" for more information about a command.''';
+  }
+
+  /// 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", "");

[Bob Nystrom, 2014/12/11 20:25:30]

This feels super sketchy to me. What's the use case for overriding `usage`?

[nweiz, 2014/12/11 23:55:24]

Usually extra bottom-matter. Pub does it to add links to the documentation site,
for instance.

[Bob Nystrom, 2014/12/12 18:13:22]

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

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

Done.

+  }
+
+  /// An unmodifiable view of all top-level commands defined for this runner.
+  Map<String, Command> get topLevelCommands =>

[Bob Nystrom, 2014/12/11 20:25:30]

I think `commands` is clear enough here. I don't think users would expect it to
flatten the tree implicitly. (The doc comment is good, though.)

[nweiz, 2014/12/11 23:55:24]

Done.

+      new UnmodifiableMapView(_topLevelCommands);
+  final _topLevelCommands = 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 topLevelArgParser = new ArgParser();

[Bob Nystrom, 2014/12/11 20:25:30]

Can this just be `argParser`? The fact that it's on CommandRunner implies
top-level to me.

[nweiz, 2014/12/11 23:55:24]

Done.

+
+  CommandRunner(this.executableName, this.description) {

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

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.

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

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.
What would we do for the usage information if it's not provided?

+    topLevelArgParser.addFlag('help', abbr: 'h', negatable: false,
+        help: 'Print this usage information.');
+    addCommand(new HelpCommand());
+  }
+
+  /// Prints the usage information for this runner.
+  ///
+  /// This is called internally by [run] and can be overridden by subclasses.

[Bob Nystrom, 2014/12/11 20:25:30]

"...to control how output is displayed" ?

[nweiz, 2014/12/11 23:55:24]

Done.

+  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) {
+      _topLevelCommands[name] = command;
+      topLevelArgParser.addCommand(name, command.argParser);

[Bob Nystrom, 2014/12/11 20:25:30]

Can we unify the parser's concept of commands with CommandRunner's? It feels
overlappy right now. For example, this will do likely bad things if the user
adds a command directly to the ArgParser.

I'd be fine with making a breaking change to the API to shuffle stuff around if
it makes for a simpler API. Alternatively, we could move CommandRunner and
Command to a completely separate library or package to make it clearer that you
can use commands in ArgParser separately from CommandRunner.

Discuss in-person?

[nweiz, 2014/12/11 23:55:24]

Moved to a separate library.

+    }
+    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 topLevelArgParser.parse(args);
+    } on FormatException catch (error) {

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

Why not just fix ArgParser.parse to throw UsageError instead of FormatException?

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

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.

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

[Bob Nystrom, 2014/12/11 20:25:30]

Can you give some details about why a user may wish to override this?

[nweiz, 2014/12/11 23:55:24]

Done.

+  Future runCommand(ArgResults topLevelOptions) {
+    return new Future.sync(() {
+      var options = topLevelOptions;
+      var commands = _topLevelCommands;
+      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]
+            .._globalOptions = topLevelOptions
+            .._options = options;

[Bob Nystrom, 2014/12/11 20:25:30]

Oh man, cascaded setters make my skin crawl.

[nweiz, 2014/12/11 23:55:24]

Done.

+        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>";

[Bob Nystrom, 2014/12/11 20:25:30]

Nit, but operators should be trailing not leading.

[nweiz, 2014/12/11 23:55:24]

Done.

+  }
+
+  /// 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;
+  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.
+  ///
+  /// 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.
+  bool get hidden {
+    // 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:31]

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

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

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

+
+  /// 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:31]

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.

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

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.
They're important at least for backwards-compatibility; they're what allowed pub
to switch from "install" to "get", for example.

+
+  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.
+  void printUsage() => print(usage);
+
+  /// Throws a [UsageException] with [message].
+  void usageError(String message) =>
+      throw new UsageException(message, _usageWithoutDescription);

[Bob Nystrom, 2014/12/11 20:25:30]

These methods are pretty similar to ones in CommandRunner. Maybe make
CommandRunner a subclass of Command?

[nweiz, 2014/12/11 23:55:24]

I think that muddies the semantic distinction too much. The executable is not
itself a command.

[Bob Nystrom, 2014/12/12 18:13:22]

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.

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

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

[Bob Nystrom, 2014/12/16 16:50:41]

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

+}
+
+/// 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.

[Bob Nystrom, 2014/12/11 20:25:30]

Do you think users will want to be able to control this order by controlling the
order they add commands?

[nweiz, 2014/12/11 23:55:24]

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.

[Bob Nystrom, 2014/12/12 18:13:22]

SGTM.

+  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();
+}