Rationalize arg handling for pub build and serve.

Rationalize arg handling for pub build and serve.

BUG=https://code.google.com/p/dart/issues/detail?id=17088
R=nweiz@google.com

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

[Bob Nystrom, 2014-03-26 21:34:09]

I still plan to reorganize the tests more, but I want to do that in a separate
patch so that it's obvious that that code doesn't need a thorough review. The
deleted/moved/added files here are more substantially different.

[nweiz, 2014-03-26 22:17:27]

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

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:47:
commandParser.addFlag("all", help: "Build all buildable directories.",
Saying "build" a bunch here is kind of confusing for "serve", and "buildable"
sounds like it's impossible to have other source directories. Maybe something
like: "Use all default source directories"?

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:63: Future
onRunTransformerCommand();
This name is kind of awkward. Can we just have the subclasses call
[super.onRun]?

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:76: void
_parseSourceDirectories() {
This method is pretty long. Maybe factor out the --all and the default directory
handling into helpers?

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:107: // general as it could
be.
Suggestion: add a [conjunction] named parameter to [toSentence] and set it to
"and/or" here.

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:108: if
(sourceDirectories.isEmpty) {
Shouldn't we return if this is non-empty? I guess there's no harm in checking
for invalid defaults, but it seems like more work than necessary.

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:132: // Make sure the build
directories don't reach out of the package.
Nit: "build directories" => "source directories"

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:157: String singularVerb,
String pluralVerb, [String suffix]) {
No need to make [suffix] optional now that it's always passed.

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/test/b...
File sdk/lib/_internal/pub/test/barback/fails_if_no_default_dirs_test.dart
(right):

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/barback/fails_if_no_default_dirs_test.dart:20: "ones
exist",
Nit: I'd indent this two or four more spaces to visually distinguish it from
another argument.

[Bob Nystrom, 2014-03-27 01:22:38]

Thanks!

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

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:47:
commandParser.addFlag("all", help: "Build all buildable directories.",
On 2014/03/26 22:17:27, nweiz wrote:
> Saying "build" a bunch here is kind of confusing for "serve", and "buildable"
> sounds like it's impossible to have other source directories. Maybe something
> like: "Use all default source directories"?

Done. I meant to fix this but must have missed it.

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:63: Future
onRunTransformerCommand();
On 2014/03/26 22:17:27, nweiz wrote:
> This name is kind of awkward. Can we just have the subclasses call
> [super.onRun]?

I always hate methods where the subclass has to remember to call the superclass
or it explodes.

(I've probably spent entirely too much time thinking about this:
http://journal.stuffwithstuff.com/2012/12/19/the-impoliteness-of-overriding-m...)

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:76: void
_parseSourceDirectories() {
On 2014/03/26 22:17:27, nweiz wrote:
> This method is pretty long. Maybe factor out the --all and the default
directory
> handling into helpers?

Done.

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:107: // general as it could
be.
On 2014/03/26 22:17:27, nweiz wrote:
> Suggestion: add a [conjunction] named parameter to [toSentence] and set it to
> "and/or" here.

There's also the "a" article at the beginning. :-/

This hack seemed like the simplest answer. Doing something general seemed like
way overkill.

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:108: if
(sourceDirectories.isEmpty) {
On 2014/03/26 22:17:27, nweiz wrote:
> Shouldn't we return if this is non-empty? I guess there's no harm in checking
> for invalid defaults, but it seems like more work than necessary.

You're right. It used to need the fall through because pub build checked for no
defaults present at the very end, but this doesn't.

Done.

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:132: // Make sure the build
directories don't reach out of the package.
On 2014/03/26 22:17:27, nweiz wrote:
> Nit: "build directories" => "source directories"

Done.

There's still a TODO in BuildDirectory to clean up all of the nomenclature. I
didn't want to do it all in this patch.

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/barback.dart:157: String singularVerb,
String pluralVerb, [String suffix]) {
On 2014/03/26 22:17:27, nweiz wrote:
> No need to make [suffix] optional now that it's always passed.

Done.

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/test/b...
File sdk/lib/_internal/pub/test/barback/fails_if_no_default_dirs_test.dart
(right):

https://codereview.chromium.org/212923006/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/barback/fails_if_no_default_dirs_test.dart:20: "ones
exist",
On 2014/03/26 22:17:27, nweiz wrote:
> Nit: I'd indent this two or four more spaces to visually distinguish it from
> another argument.

Done.

[nweiz, 2014-03-27 01:35:14]

lgtm

[Bob Nystrom, 2014-03-28 22:52:54]

Committed patchset #2 manually as r34539 (presubmit successful).