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..2a4d13e29ab41fd524f9023a214dc04bfb25f836 100644
--- a/sdk/lib/_internal/pub/lib/src/command.dart
+++ b/sdk/lib/_internal/pub/lib/src/command.dart
@@ -33,7 +33,7 @@ import 'utils.dart';
 /// The base class for commands for the pub executable.

[nweiz, 2014/01/31 21:42:06]

Explain how commands vs subcommands work.

[Bob Nystrom, 2014/02/01 01:49:32]

Done.

 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 +44,45 @@ 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());
+    listCommands(mainCommands, buffer);
     buffer.writeln();
+    buffer.writeln(
+        'Use "pub help [command]" for more information about a command.');
+
+    log.message(buffer);
+  }
 
+  /// Writes [commands] in a nicely formatted list to [buffer].

[nweiz, 2014/01/31 21:42:06]

Document [isSubcommand].

[Bob Nystrom, 2014/02/01 01:49:32]

Done.

+  static void listCommands(Map<String, PubCommand> commands,
+                           StringBuffer buffer, {bool isSubcommand: false}) {

[nweiz, 2014/01/31 21:42:06]

I really don't like passing in a StringBuffer as opposed to returning a string.
There's no way the marginal performance benefit outweighs the ugliness of the
signature.

Rather than passing both [commands] and [isSubcommand], why not just pass the
[PubCommand]? Then you can detect whether it's a subcommand by checking it
against [mainCommands].

[Bob Nystrom, 2014/02/01 01:49:32]

Done.

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

[nweiz, 2014/01/31 21:42:06]

I don't think you should remove this even if there aren't currently any
applicable commands. It's still useful to have hidden-ness be transitive in this
way in case we add future hidden commands.

[Bob Nystrom, 2014/02/01 01:49:32]

Yup. It turns out this needs to stay to handle another error case too.

+    if (names.isEmpty) return;
+
     var length = names.map((name) => name.length).reduce(math.max);
 
-    buffer.writeln('Available commands:');
+    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());
   }
 
   SystemCache cache;
 
   /// The parsed options for this command.
-  ArgResults commandOptions;
+  ArgResults get commandOptions => _commandOptions;
+  ArgResults _commandOptions;
 
   Entrypoint entrypoint;
 
@@ -102,6 +112,13 @@ 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.
@@ -113,13 +130,10 @@ 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.

[nweiz, 2014/01/31 21:42:06]

Document the arguments. It's especially unclear what the difference between
[mainOptions] and [commandOptions] is.

I also think [mainOptions] could have a clearer name. Maybe "globalOptions"?

[Bob Nystrom, 2014/02/01 01:49:32]

Documented. "global" is a little misleading because it includes all of the
command options too, not just the top-level ones. Changed to "rootOptions".

+  void run(String cacheDir, ArgResults mainOptions, ArgResults commandOptions,
+           List<String> arguments) {
+    _commandOptions = commandOptions;
 
     cache = new SystemCache.withSources(cacheDir, isOffline: isOffline);
 
@@ -131,7 +145,7 @@ abstract class PubCommand {
       log.error(getErrorMessage(error));
       log.fine("Exception type: ${error.runtimeType}");
 
-      if (options['trace'] || !isUserFacingException(error)) {
+      if (mainOptions['trace'] || !isUserFacingException(error)) {
         log.error(chain.terse);
       } else {
         log.fine(chain.terse);
@@ -143,7 +157,7 @@ abstract class PubCommand {
         log.fine(message);
       }
 
-      if (options['trace']) {
+      if (mainOptions['trace']) {
         log.dumpTranscript();
       } else if (!isUserFacingException(error)) {
         log.error("""
@@ -159,7 +173,10 @@ and include the results in a bug report on http://dartbug.com/new.
     }
 
     var captureStackChains =
-        options['trace'] || options['verbose'] || options['verbosity'] == 'all';
+        mainOptions['trace'] ||
+        mainOptions['verbose'] ||
+        mainOptions['verbosity'] == 'all';
+
     captureErrors(() {
       return syncFuture(() {
         // Make sure there aren't unexpected arguments.
@@ -193,7 +210,10 @@ and include the results in a bug report on http://dartbug.com/new.
   /// 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`.

[nweiz, 2014/01/31 21:42:06]

Mention that this only needs to be overridden for leaf commands.

[Bob Nystrom, 2014/02/01 01:49:32]

Done.

-  Future onRun();
+  Future onRun() {
+    // Leaf commands should override this.

[nweiz, 2014/01/31 21:42:06]

For non-leaf commands, this should still never be called, right?

[Bob Nystrom, 2014/02/01 01:49:32]

Yup. Changed to assert(false).

+    assert(subcommands.isNotEmpty);
+  }
 
   /// Displays usage information for this command.
   void printUsage([String description]) {
@@ -208,6 +228,11 @@ and include the results in a bug report on http://dartbug.com/new.
       buffer.write(commandUsage);
     }
 
+    if (subcommands.isNotEmpty) {
+      buffer.writeln();
+      listCommands(subcommands, buffer);
+    }
+
     log.message(buffer.toString());
   }
 
@@ -272,9 +297,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);
+  });
+}