Support subcommands in pub and pub help.

Support subcommands in pub and pub help.

BUG=
R=nweiz@google.com

Committed: https://code.google.com/p/dart/source/detail?r=32376

[nweiz, 2014-01-31 21:42:05]

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/bin/pu...
File sdk/lib/_internal/pub/bin/pub.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/bin/pu...
sdk/lib/_internal/pub/bin/pub.dart:72: validatePlatform().then((_) {
Nit: =>

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/bin/pu...
sdk/lib/_internal/pub/bin/pub.dart:77: void runCommand(String cacheDir,
ArgResults mainOptions,
Document this.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/bin/pu...
sdk/lib/_internal/pub/bin/pub.dart:109: _usageError('Could not find a command
named "${options.rest[0]}".');
"find" seems weird here -- it's not like the command is somewhere and we
misplaced it, it just doesn't exist.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:33: /// The base class for commands
for the pub executable.
Explain how commands vs subcommands work.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:59: /// Writes [commands] in a nicely
formatted list to [buffer].
Document [isSubcommand].

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:61: StringBuffer buffer, {bool
isSubcommand: false}) {
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].

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:68: // TODO(rnystrom): Remove this
once cache has a visible command.
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.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:94: bool get hidden => false;
Document that this only needs to be set for leaf nodes. Same for
[requiresEntrypoint], [takesArguments], and [isOffline].

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:133: /// Runs this command.
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"?

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:212: /// command is synchronous, it
may return `null`.
Mention that this only needs to be overridden for leaf commands.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:214: // Leaf commands should override
this.
For non-leaf commands, this should still never be called, right?

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:30: String get description =>
"Apply transformers to build a package.";
This CL isn't really a good place for this change.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/cache.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/cache.dart:15: bool get hidden => true;
It shouldn't be necessary to set [hidden] or [requiresEntrypoint] for non-leaf
commands.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/cache_list.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/cache_list.dart:1: // Copyright (c) 2013,
the Dart project authors.  Please see the AUTHORS file
I feel like this should be command/cache/list.dart.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/cache_list.dart:15: String get description
=> "List packages in system cache.";
"in system" -> "in the system"

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/help.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/help.dart:34: _usageError(message) {
You've defined this same function here and in pub.dart. Maybe
[PubCommand.listCommands] should be private and this should be
[PubCommand.usageError]?

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/help.dart:50: return _usageError('Could
not find a command named "$name".');
Same issue with "find" as before.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/log.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:126: }
I like the effect of this, but I don't like the logic. We should just always get
rid of any trailing whitespace; checking whether [message] is a string buffer is
a hack.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/test/p...
File sdk/lib/_internal/pub/test/pub_test.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/test/p...
sdk/lib/_internal/pub/test/pub_test.dart:84: integration('running pub with
--help after command with subcommands shows '
"after command" -> "after a command"

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/test/p...
sdk/lib/_internal/pub/test/pub_test.dart:169: Missing subcommand for "pub
cache".
This should also mention "Usage: pub cache <command>"

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/test/p...
sdk/lib/_internal/pub/test/pub_test.dart:249: Command "pub version" does not
expect a subcommand.
This should print the documentation for "pub version".

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

Thanks!

Your comments triggered a bit of a cascade of changes, but I like where it ended
up. PTAL!

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/bin/pu...
File sdk/lib/_internal/pub/bin/pub.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/bin/pu...
sdk/lib/_internal/pub/bin/pub.dart:72: validatePlatform().then((_) {
On 2014/01/31 21:42:06, nweiz wrote:
> Nit: =>

Done.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/bin/pu...
sdk/lib/_internal/pub/bin/pub.dart:77: void runCommand(String cacheDir,
ArgResults mainOptions,
On 2014/01/31 21:42:06, nweiz wrote:
> Document this.

Done.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/bin/pu...
sdk/lib/_internal/pub/bin/pub.dart:109: _usageError('Could not find a command
named "${options.rest[0]}".');
On 2014/01/31 21:42:06, nweiz wrote:
> "find" seems weird here -- it's not like the command is somewhere and we
> misplaced it, it just doesn't exist.

<shrug> It's the same message we had before. I think it's a more polite way than
saying "you're wrong!"

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:33: /// The base class for commands
for the pub executable.
On 2014/01/31 21:42:06, nweiz wrote:
> Explain how commands vs subcommands work.

Done.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:59: /// Writes [commands] in a nicely
formatted list to [buffer].
On 2014/01/31 21:42:06, nweiz wrote:
> Document [isSubcommand].

Done.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:61: StringBuffer buffer, {bool
isSubcommand: false}) {
On 2014/01/31 21:42:06, nweiz wrote:
> 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].

Done.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:68: // TODO(rnystrom): Remove this
once cache has a visible command.
On 2014/01/31 21:42:06, nweiz wrote:
> 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.

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

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:94: bool get hidden => false;
On 2014/01/31 21:42:06, nweiz wrote:
> Document that this only needs to be set for leaf nodes. Same for
> [requiresEntrypoint], [takesArguments], and [isOffline].

hidden is actually meaningful for parent commands too, but done for the others.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:133: /// Runs this command.
On 2014/01/31 21:42:06, nweiz wrote:
> 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"?

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

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:212: /// command is synchronous, it
may return `null`.
On 2014/01/31 21:42:06, nweiz wrote:
> Mention that this only needs to be overridden for leaf commands.

Done.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:214: // Leaf commands should override
this.
On 2014/01/31 21:42:06, nweiz wrote:
> For non-leaf commands, this should still never be called, right?

Yup. Changed to assert(false).

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:30: String get description =>
"Apply transformers to build a package.";
On 2014/01/31 21:42:06, nweiz wrote:
> This CL isn't really a good place for this change.

Eh, I noticed it. Does it hurt?

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/cache.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/cache.dart:15: bool get hidden => true;
On 2014/01/31 21:42:06, nweiz wrote:
> It shouldn't be necessary to set [hidden] or [requiresEntrypoint] for non-leaf
> commands.

Hidden is still useful. Removed requiresEntrypoint.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/cache_list.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/cache_list.dart:1: // Copyright (c) 2013,
the Dart project authors.  Please see the AUTHORS file
On 2014/01/31 21:42:06, nweiz wrote:
> I feel like this should be command/cache/list.dart.

We don't have many subcommands, so that feels a bit gratuitous to me. I'm up for
reorganizing it like that if the command directory gets unmanageable.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/cache_list.dart:15: String get description
=> "List packages in system cache.";
On 2014/01/31 21:42:06, nweiz wrote:
> "in system" -> "in the system"

Done.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/help.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/help.dart:34: _usageError(message) {
On 2014/01/31 21:42:06, nweiz wrote:
> You've defined this same function here and in pub.dart. Maybe
> [PubCommand.listCommands] should be private and this should be
> [PubCommand.usageError]?

OK, done. This triggered a cascade of changes. To make usageError more usage, I
made it throw. In order for that to work in bin/pub.dart, I had to move the
error-handling in there to ensure it catches the UsageException.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/help.dart:50: return _usageError('Could
not find a command named "$name".');
On 2014/01/31 21:42:06, nweiz wrote:
> Same issue with "find" as before.

Same explanation as before. :)

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/log.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:126: }
On 2014/01/31 21:42:06, nweiz wrote:
> I like the effect of this, but I don't like the logic. We should just always
get
> rid of any trailing whitespace; checking whether [message] is a string buffer
is
> a hack.

Done. I tried that at first but it broke the pub lish tests. The confirmation
prompt confuses log a bit. Fixed that in pub lish.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/test/p...
File sdk/lib/_internal/pub/test/pub_test.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/test/p...
sdk/lib/_internal/pub/test/pub_test.dart:84: integration('running pub with
--help after command with subcommands shows '
On 2014/01/31 21:42:06, nweiz wrote:
> "after command" -> "after a command"

Done.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/test/p...
sdk/lib/_internal/pub/test/pub_test.dart:169: Missing subcommand for "pub
cache".
On 2014/01/31 21:42:06, nweiz wrote:
> This should also mention "Usage: pub cache <command>"

Done.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/test/p...
sdk/lib/_internal/pub/test/pub_test.dart:249: Command "pub version" does not
expect a subcommand.
On 2014/01/31 21:42:06, nweiz wrote:
> This should print the documentation for "pub version".

Done.

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

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:94: bool get hidden => false;
On 2014/02/01 01:49:32, Bob Nystrom wrote:
> On 2014/01/31 21:42:06, nweiz wrote:
> > Document that this only needs to be set for leaf nodes. Same for
> > [requiresEntrypoint], [takesArguments], and [isOffline].
> 
> hidden is actually meaningful for parent commands too, but done for the
others.

What's the benefit to having a parent command's hidden status be explicitly
configured, as opposed to saying that it's hidden if and only if all its
children are hidden?

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:30: String get description =>
"Apply transformers to build a package.";
On 2014/02/01 01:49:32, Bob Nystrom wrote:
> On 2014/01/31 21:42:06, nweiz wrote:
> > This CL isn't really a good place for this change.
> 
> Eh, I noticed it. Does it hurt?

I like keeping separate changes separate because sometimes separate operations
are done on them -- e.g. if one needs to be reverted, the other has to be along
with it.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/b...
File sdk/lib/_internal/pub/bin/pub.dart (right):

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/b...
sdk/lib/_internal/pub/bin/pub.dart:77: /// Runs the appropriate pub command
parsed to [options] from [arguments] using
I'm not sure what "parsed to [options] from [arguments]" means.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/b...
sdk/lib/_internal/pub/bin/pub.dart:82: handleError(error, Chain chain) {
I don't like how this is a separate named function outside of the normal flow of
the code. I think we originally did that because it was called multiple times,
but it's only used once now so let's move it back into [catchError].

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/b...
sdk/lib/_internal/pub/bin/pub.dart:123: return invokeCommand(cacheDir, options);
Nit: =>

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/b...
sdk/lib/_internal/pub/bin/pub.dart:160: return null;
I know it works in this case, but I don't like returning null from functions
that declare Future. It makes it harder to re-use or move the function somewhere
else, since [then] won't work.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/b...
sdk/lib/_internal/pub/bin/pub.dart:172: 'for "$commandString".');
Nit: when writing multiline strings like this, I like to add an extra level of
indentation when the previous line didn't end in a newline. This helps visually
differentiate actual newlines in the text from newlines imposed by line-length
limitations.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
sdk/lib/_internal/pub/lib/src/command.dart:60: static void
commandUsageError(Map<String, PubCommand> commands,
"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"?

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
sdk/lib/_internal/pub/lib/src/command.dart:68: /// "subcommands", otherwise they
are labelled "commands".
[isSubcommand] isn't passed anymore.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
sdk/lib/_internal/pub/lib/src/command.dart:151: Future run(String cacheDir,
ArgResults options) {
What happened to mainOptions/rootOptions? Subcommands will want to have access
to the parent commands' options.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
sdk/lib/_internal/pub/lib/src/command.dart:164: if (commandFuture == null)
return null;
Why not just "return syncFuture(onRun)"?

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
sdk/lib/_internal/pub/lib/src/command.dart:173: /// lead command should override
this.
"lead" -> "leaf".

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
sdk/lib/_internal/pub/lib/src/command.dart:203: }
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:30]

Thanks!

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:94: bool get hidden => false;
On 2014/02/04 01:24:29, nweiz wrote:
> On 2014/02/01 01:49:32, Bob Nystrom wrote:
> > On 2014/01/31 21:42:06, nweiz wrote:
> > > Document that this only needs to be set for leaf nodes. Same for
> > > [requiresEntrypoint], [takesArguments], and [isOffline].
> > 
> > hidden is actually meaningful for parent commands too, but done for the
> others.
> 
> What's the benefit to having a parent command's hidden status be explicitly
> configured, as opposed to saying that it's hidden if and only if all its
> children are hidden?

It lets you do:

$ pub help
// no method of pub cache because it's hidden...

$ pub help cache
// see help for the "add" command

In other words, if you happen to know a hidden command exists, you can see into
it and its child commands.

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:30: String get description =>
"Apply transformers to build a package.";
On 2014/02/04 01:24:29, nweiz wrote:
> On 2014/02/01 01:49:32, Bob Nystrom wrote:
> > On 2014/01/31 21:42:06, nweiz wrote:
> > > This CL isn't really a good place for this change.
> > 
> > Eh, I noticed it. Does it hurt?
> 
> I like keeping separate changes separate because sometimes separate operations
> are done on them -- e.g. if one needs to be reverted, the other has to be
along
> with it.

Yeah, that makes sense. I <3 nice small orthogonal patches too. I threw this in
here because I knew I'd forget to fix it later since it's so trivial.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/b...
File sdk/lib/_internal/pub/bin/pub.dart (right):

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/b...
sdk/lib/_internal/pub/bin/pub.dart:77: /// Runs the appropriate pub command
parsed to [options] from [arguments] using
On 2014/02/04 01:24:29, nweiz wrote:
> I'm not sure what "parsed to [options] from [arguments]" means.

Reworded.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/b...
sdk/lib/_internal/pub/bin/pub.dart:82: handleError(error, Chain chain) {
On 2014/02/04 01:24:29, nweiz wrote:
> I don't like how this is a separate named function outside of the normal flow
of
> the code. I think we originally did that because it was called multiple times,
> but it's only used once now so let's move it back into [catchError].

Done.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/b...
sdk/lib/_internal/pub/bin/pub.dart:123: return invokeCommand(cacheDir, options);
On 2014/02/04 01:24:29, nweiz wrote:
> Nit: =>

Done.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/b...
sdk/lib/_internal/pub/bin/pub.dart:160: return null;
On 2014/02/04 01:24:29, nweiz wrote:
> I know it works in this case, but I don't like returning null from functions
> that declare Future. It makes it harder to re-use or move the function
somewhere
> else, since [then] won't work.

Done.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/b...
sdk/lib/_internal/pub/bin/pub.dart:172: 'for "$commandString".');
On 2014/02/04 01:24:29, nweiz wrote:
> Nit: when writing multiline strings like this, I like to add an extra level of
> indentation when the previous line didn't end in a newline. This helps
visually
> differentiate actual newlines in the text from newlines imposed by line-length
> limitations.

Moved the first line up. I think that makes it consistent with other multiline
literals. We have lots of code like:

blah("some string that has no line break "
    "the rest of the string");

Where the second line is only +4. Made this match.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
sdk/lib/_internal/pub/lib/src/command.dart:60: static void
commandUsageError(Map<String, PubCommand> commands,
On 2014/02/04 01:24:29, nweiz wrote:
> "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"?

Done.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
sdk/lib/_internal/pub/lib/src/command.dart:68: /// "subcommands", otherwise they
are labelled "commands".
On 2014/02/04 01:24:29, nweiz wrote:
> [isSubcommand] isn't passed anymore.

Done.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
sdk/lib/_internal/pub/lib/src/command.dart:151: Future run(String cacheDir,
ArgResults options) {
On 2014/02/04 01:24:29, nweiz wrote:
> What happened to mainOptions/rootOptions? Subcommands will want to have access
> to the parent commands' options.

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.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
sdk/lib/_internal/pub/lib/src/command.dart:164: if (commandFuture == null)
return null;
On 2014/02/04 01:24:29, nweiz wrote:
> Why not just "return syncFuture(onRun)"?

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

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
sdk/lib/_internal/pub/lib/src/command.dart:173: /// lead command should override
this.
On 2014/02/04 01:24:29, nweiz wrote:
> "lead" -> "leaf".

Done.

https://codereview.chromium.org/138723005/diff/120001/sdk/lib/_internal/pub/l...
sdk/lib/_internal/pub/lib/src/command.dart:203: }
On 2014/02/04 01:24:29, nweiz wrote:
> Nit: it reads weird to me to have this defined between [printUsage] and
> [usageError], as opposed to after [usageError].

Done.

[nweiz, 2014-02-06 01:57:40]

lgtm

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:94: bool get hidden => false;
On 2014/02/06 00:06:31, Bob Nystrom wrote:
> On 2014/02/04 01:24:29, nweiz wrote:
> > On 2014/02/01 01:49:32, Bob Nystrom wrote:
> > > On 2014/01/31 21:42:06, nweiz wrote:
> > > > Document that this only needs to be set for leaf nodes. Same for
> > > > [requiresEntrypoint], [takesArguments], and [isOffline].
> > > 
> > > hidden is actually meaningful for parent commands too, but done for the
> > others.
> > 
> > What's the benefit to having a parent command's hidden status be explicitly
> > configured, as opposed to saying that it's hidden if and only if all its
> > children are hidden?
> 
> It lets you do:
> 
> $ pub help
> // no method of pub cache because it's hidden...
> 
> $ pub help cache
> // see help for the "add" command
> 
> In other words, if you happen to know a hidden command exists, you can see
into
> it and its child commands.

It seems to me that this is all derivable from the leaf nodes' hiddenness. "pub
help" wouldn't list cache because it has no visible children, but "pub help
cache" would list its children because *all* of them are hidden.

[Bob Nystrom, 2014-02-06 18:35:02]

On 2014/02/06 01:57:40, nweiz wrote:
> lgtm
> 
>
https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
> File sdk/lib/_internal/pub/lib/src/command.dart (right):
> 
>
https://codereview.chromium.org/138723005/diff/1/sdk/lib/_internal/pub/lib/sr...
> sdk/lib/_internal/pub/lib/src/command.dart:94: bool get hidden => false;
> On 2014/02/06 00:06:31, Bob Nystrom wrote:
> > On 2014/02/04 01:24:29, nweiz wrote:
> > > On 2014/02/01 01:49:32, Bob Nystrom wrote:
> > > > On 2014/01/31 21:42:06, nweiz wrote:
> > > > > Document that this only needs to be set for leaf nodes. Same for
> > > > > [requiresEntrypoint], [takesArguments], and [isOffline].
> > > > 
> > > > hidden is actually meaningful for parent commands too, but done for the
> > > others.
> > > 
> > > What's the benefit to having a parent command's hidden status be
explicitly
> > > configured, as opposed to saying that it's hidden if and only if all its
> > > children are hidden?
> > 
> > It lets you do:
> > 
> > $ pub help
> > // no method of pub cache because it's hidden...
> > 
> > $ pub help cache
> > // see help for the "add" command
> > 
> > In other words, if you happen to know a hidden command exists, you can see
> into
> > it and its child commands.
> 
> It seems to me that this is all derivable from the leaf nodes' hiddenness.
"pub
> help" wouldn't list cache because it has no visible children, but "pub help
> cache" would list its children because *all* of them are hidden.

Done.

[Bob Nystrom, 2014-02-06 18:52:09]

Committed patchset #4 manually as r32376 (presubmit successful).