Support subcommands in pub and pub help.

sdk/lib/_internal/pub/lib/src/command.dart

Index: sdk/lib/_internal/pub/lib/src/command.dart
diff --git a/sdk/lib/_internal/pub/lib/src/command.dart b/sdk/lib/_internal/pub/lib/src/command.dart
index e3dcab5ef072fac8cfb9c72cf00217044dcaeee4..145ce01b181c9c5956571fc693689a90b9c47bdb 100644
--- a/sdk/lib/_internal/pub/lib/src/command.dart
+++ b/sdk/lib/_internal/pub/lib/src/command.dart
@@ -5,12 +5,10 @@
 library pub.command;
 
 import 'dart:async';
-import 'dart:io';
 import 'dart:math' as math;
 
 import 'package:args/args.dart';
 import 'package:path/path.dart' as path;
-import 'package:stack_trace/stack_trace.dart';
 
 import 'command/build.dart';
 import 'command/cache.dart';
@@ -23,17 +21,18 @@ import 'command/upgrade.dart';
 import 'command/uploader.dart';
 import 'command/version.dart';
 import 'entrypoint.dart';
-import 'exit_codes.dart' as exit_codes;
-import 'http.dart';
-import 'io.dart';
 import 'log.dart' as log;
 import 'system_cache.dart';
 import 'utils.dart';
 
 /// The base class for commands for the pub executable.
+///
+/// A command may either be a "leaf" command or it may be a parent for a set
+/// of subcommands. Only leaf commands are ever actually invoked. If a command
+/// has subcommands, then one of those must always be chosen.
 abstract class PubCommand {
   /// The commands that pub understands.
-  static final Map<String, PubCommand> commands = _initCommands();
+  static final Map<String, PubCommand> mainCommands = _initCommands();
 
   /// The top-level [ArgParser] used to parse the pub command line.
   static final pubArgParser = _initArgParser();
@@ -44,35 +43,59 @@ abstract class PubCommand {
     var buffer = new StringBuffer();
     buffer.writeln('Pub is a package manager for Dart.');
     buffer.writeln();
-    buffer.writeln('Usage: pub command [arguments]');
+    buffer.writeln('Usage: pub <command> [arguments]');
     buffer.writeln();
     buffer.writeln('Global options:');
     buffer.writeln(pubArgParser.getUsage());
+    buffer.write(_listCommands(mainCommands));
     buffer.writeln();
+    buffer.writeln(
+        'Use "pub help [command]" for more information about a command.');
+
+    log.message(buffer);
+  }
+
+  /// Fails with a usage error [message] when trying to select from one of
+  /// [commands].
+  static void commandUsageError(Map<String, PubCommand> commands,

[nweiz, 2014/02/04 01:24:29]

"PubCommand.commandUsageError" reads weird, both because you write "command"
twice and because it's not clear that it means "usage error that lists available
commands". Maybe "usageErrorWithCommands"?

[Bob Nystrom, 2014/02/06 00:06:31]

Done.

+                                String message) {
+    throw new UsageException("$message\n${_listCommands(commands)}");
+  }
 
+  /// Writes [commands] in a nicely formatted list to [buffer].
+  ///
+  /// If [isSubcommand] is `true`, then the list will be labelled as
+  /// "subcommands", otherwise they are labelled "commands".

[nweiz, 2014/02/04 01:24:29]

[isSubcommand] isn't passed anymore.

[Bob Nystrom, 2014/02/06 00:06:31]

Done.

+  static String _listCommands(Map<String, PubCommand> commands) {
     // Show the public commands alphabetically.
     var names = ordered(commands.keys.where((name) =>
         !commands[name].aliases.contains(name) &&
         !commands[name].hidden));
 
+    // If all of the subcommands are hidden, do nothing.
+    // TODO(rnystrom): Remove this once cache has a visible command.
+    if (names.isEmpty) return "";
+
     var length = names.map((name) => name.length).reduce(math.max);
 
-    buffer.writeln('Available commands:');
+    var isSubcommand = commands != mainCommands;
+
+    var buffer = new StringBuffer();
+    buffer.writeln();
+    buffer.writeln('Available ${isSubcommand ? "sub" : ""}commands:');
     for (var name in names) {
       buffer.writeln('  ${padRight(name, length)}   '
           '${commands[name].description}');
     }
 
-    buffer.writeln();
-    buffer.write(
-        'Use "pub help [command]" for more information about a command.');
-    log.message(buffer.toString());
+    return buffer.toString();
   }
 
   SystemCache cache;
 
   /// The parsed options for this command.
-  ArgResults commandOptions;
+  ArgResults get commandOptions => _commandOptions;
+  ArgResults _commandOptions;
 
   Entrypoint entrypoint;
 
@@ -86,13 +109,16 @@ abstract class PubCommand {
   /// How to invoke this command (e.g. `"pub get [package]"`).
   String get usage;
 
-  /// Whether or not this command requires [entrypoint] to be defined. If false,
-  /// pub won't look for a pubspec and [entrypoint] will be null when the
-  /// command runs.
+  /// Whether or not this command requires [entrypoint] to be defined.
+  ///
+  /// If false, pub won't look for a pubspec and [entrypoint] will be null when
+  /// the command runs. This only needs to be set in leaf commands.
   bool get requiresEntrypoint => true;
 
-  /// Whether or not this command takes arguments in addition to options. If
-  /// false, pub will exit with an error if arguments are provided.
+  /// Whether or not this command takes arguments in addition to options.
+  ///
+  /// If false, pub will exit with an error if arguments are provided. This
+  /// only needs to be set in leaf commands.
   bool get takesArguments => false;
 
   /// Alternate names for this command. These names won't be used in the
@@ -102,9 +128,17 @@ abstract class PubCommand {
   /// The [ArgParser] for this command.
   final commandParser = new ArgParser();
 
+  /// Subcommands exposed by this command.
+  ///
+  /// If empty, then this command has no subcommands. Otherwise, a subcommand
+  /// must be specified by the user. In that case, this command's [onRun] will
+  /// not be called and the subcommand's will.
+  final subcommands = <String, PubCommand>{};
+
   /// Override this to use offline-only sources instead of hitting the network.
+  ///
   /// This will only be called before the [SystemCache] is created. After that,
-  /// it has no effect.
+  /// it has no effect. This only needs to be set in leaf commands.
   bool get isOffline => false;
 
   PubCommand() {
@@ -113,94 +147,46 @@ abstract class PubCommand {
         help: 'Print usage information for this command.');
   }
 
-  void run(String cacheDir, ArgResults options, List<String> arguments) {
-    commandOptions = options.command;
-
-    if (commandOptions['help']) {
-      this.printUsage();
-      return;
-    }
+  /// Runs this command using a system cache at [cacheDir] with [options].
+  Future run(String cacheDir, ArgResults options) {

[nweiz, 2014/02/04 01:24:29]

What happened to mainOptions/rootOptions? Subcommands will want to have access
to the parent commands' options.

[Bob Nystrom, 2014/02/06 00:06:31]

I took it out because it wasn't being used. We probably will need it at some
point, but no commands do right now. I'll add it back in once a command requires
it.

+    _commandOptions = options;
 
     cache = new SystemCache.withSources(cacheDir, isOffline: isOffline);
 
-    handleError(error, Chain chain) {
-      // This is basically the top-level exception handler so that we don't
-      // spew a stack trace on our users.
-      var message;
-
-      log.error(getErrorMessage(error));
-      log.fine("Exception type: ${error.runtimeType}");
-
-      if (options['trace'] || !isUserFacingException(error)) {
-        log.error(chain.terse);
-      } else {
-        log.fine(chain.terse);
-      }
-
-      if (error is ApplicationException && error.innerError != null) {
-        var message = "Wrapped exception: ${error.innerError}";
-        if (error.innerTrace != null) message = "$message\n${error.innerTrace}";
-        log.fine(message);
-      }
-
-      if (options['trace']) {
-        log.dumpTranscript();
-      } else if (!isUserFacingException(error)) {
-        log.error("""
-This is an unexpected error. Please run
-
-    pub --trace ${arguments.map((arg) => "'$arg'").join(' ')}
-
-and include the results in a bug report on http://dartbug.com/new.
-""");
-      }
-
-      return flushThenExit(_chooseExitCode(error));
+    if (requiresEntrypoint) {
+      // TODO(rnystrom): Will eventually need better logic to walk up
+      // subdirectories until we hit one that looks package-like. For now,
+      // just assume the cwd is it.
+      entrypoint = new Entrypoint(path.current, cache);
     }
 
-    var captureStackChains =
-        options['trace'] || options['verbose'] || options['verbosity'] == 'all';
-    captureErrors(() {
-      return syncFuture(() {
-        // Make sure there aren't unexpected arguments.
-        if (!takesArguments && commandOptions.rest.isNotEmpty) {
-          log.error('Command "${commandOptions.name}" does not take any '
-                    'arguments.');
-          this.printUsage();
-          return flushThenExit(exit_codes.USAGE);
-        }
-
-        if (requiresEntrypoint) {
-          // TODO(rnystrom): Will eventually need better logic to walk up
-          // subdirectories until we hit one that looks package-like. For now,
-          // just assume the cwd is it.
-          entrypoint = new Entrypoint(path.current, cache);
-        }
-
-        var commandFuture = onRun();
-        if (commandFuture == null) return true;
-
-        return commandFuture;
-      }).whenComplete(() => cache.deleteTempDir());
-    }, captureStackChains: captureStackChains).catchError(handleError)
-        .then((_) {
-      // Explicitly exit on success to ensure that any dangling dart:io handles
-      // don't cause the process to never terminate.
-      return flushThenExit(exit_codes.SUCCESS);
-    });
+    var commandFuture = onRun();
+    if (commandFuture == null) return null;

[nweiz, 2014/02/04 01:24:29]

Why not just "return syncFuture(onRun)"?

[Bob Nystrom, 2014/02/06 00:06:31]

Done, though that behavior is a bit different. But not in a way that matters.

+
+    return commandFuture;
   }
 
-  /// Override this to perform the specific command. Return a future that
-  /// completes when the command is done or fails if the command fails. If the
-  /// command is synchronous, it may return `null`.
-  Future onRun();
+  /// Override this to perform the specific command.
+  ///
+  /// Return a future that completes when the command is done or fails if the
+  /// command fails. If the command is synchronous, it may return `null`. Only
+  /// lead command should override this.

[nweiz, 2014/02/04 01:24:29]

"lead" -> "leaf".

[Bob Nystrom, 2014/02/06 00:06:31]

Done.

+  Future onRun() {
+    // Leaf commands should override this and non-leaf commands should never
+    // call it.
+    assert(false);
+  }
 
   /// Displays usage information for this command.
   void printUsage([String description]) {
     if (description == null) description = this.description;
+    log.message('$description\n\n${_getUsage()}');
+  }
 
+  /// Generates a string of usage information for this command.
+  String _getUsage() {
     var buffer = new StringBuffer();
-    buffer.write('$description\n\nUsage: $usage');
+    buffer.write('Usage: $usage');
 
     var commandUsage = commandParser.getUsage();
     if (!commandUsage.isEmpty) {
@@ -208,20 +194,19 @@ and include the results in a bug report on http://dartbug.com/new.
       buffer.write(commandUsage);
     }
 
-    log.message(buffer.toString());
+    if (subcommands.isNotEmpty) {
+      buffer.writeln();
+      buffer.write(_listCommands(subcommands));
+    }
+
+    return buffer.toString();
   }

[nweiz, 2014/02/04 01:24:29]

Nit: it reads weird to me to have this defined between [printUsage] and
[usageError], as opposed to after [usageError].

[Bob Nystrom, 2014/02/06 00:06:31]

Done.

 
-  /// Returns the appropriate exit code for [exception], falling back on 1 if no
-  /// appropriate exit code could be found.
-  int _chooseExitCode(exception) {
-    if (exception is HttpException || exception is HttpException ||
-        exception is SocketException || exception is PubHttpException) {
-      return exit_codes.UNAVAILABLE;
-    } else if (exception is FormatException) {
-      return exit_codes.DATA;
-    } else {
-      return 1;
-    }
+  // TODO(rnystrom): Use this in other places handle usage failures.
+  /// Throw an [ApplicationException] for a usage error of this command with
+  /// [message].
+  void usageError(String message) {
+    throw new UsageException("$message\n\n${_getUsage()}");
   }
 }
 
@@ -272,9 +257,19 @@ ArgParser _initArgParser() {
       help: 'Shortcut for "--verbosity=all".');
 
   // Register the commands.
-  PubCommand.commands.forEach((name, command) {
-    argParser.addCommand(name, command.commandParser);
+  PubCommand.mainCommands.forEach((name, command) {
+    _registerCommand(name, command, argParser);
   });
 
   return argParser;
 }
+
+/// Registers a [command] with [name] on [parser].
+void _registerCommand(String name, PubCommand command, ArgParser parser) {
+  parser.addCommand(name, command.commandParser);
+
+  // Recursively wire up any subcommands.
+  command.subcommands.forEach((name, subcommand) {
+    _registerCommand(name, subcommand, command.commandParser);
+  });
+}