Fix ALL the pub tests.

Fix ALL the pub tests.

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

[nweiz, 2013-01-09 06:14:37]

This includes https://codereview.chromium.org/11818013/ and
https://codereview.chromium.org/11821018/. You should be able to just ignore all
the changes outside of util/.

[Bob Nystrom, 2013-01-09 16:49:23]

Nits and suggestions. Otherwise LGTM. Woo!

https://codereview.chromium.org/11830017/diff/1/utils/pub/git.dart
File utils/pub/git.dart (right):

https://codereview.chromium.org/11830017/diff/1/utils/pub/git.dart#newcode75
utils/pub/git.dart:75: });
Much nicer!

https://codereview.chromium.org/11830017/diff/1/utils/pub/hosted_source.dart
File utils/pub/hosted_source.dart (right):

https://codereview.chromium.org/11830017/diff/1/utils/pub/hosted_source.dart#...
utils/pub/hosted_source.dart:122: void _throwFriendlyError(asyncError, package,
url) {
Can we type annotate this too?

https://codereview.chromium.org/11830017/diff/1/utils/pub/hosted_source.dart#...
utils/pub/hosted_source.dart:139: throw asyncError;
Argh good catch. Completely misleading comment on my part. :(

https://codereview.chromium.org/11830017/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/11830017/diff/1/utils/pub/io.dart#newcode197
utils/pub/io.dart:197: log.fine("Got 'already exists' error when creating
directory.");
This log is wrong. This "if" is for when the error is *not* an "already exists"
one. We should probably negate the condition:

if (error.osError.errorCode == 17 ||
    error.osError.errorCode == 183) {
  log.fine("Got 'already exists' error when creating directory.");
  return _getDirectory(path);
}

throw asyncError;

https://codereview.chromium.org/11830017/diff/1/utils/pub/io.dart#newcode567
utils/pub/io.dart:567: Stream<List<int>> wrapInputStream(InputStream stream) {
Type the return as a ByteStream?

Also, can we have a more explicit name for this? At the call sites, it wasn't
clear to me what it was wrapping the input stream in. Is this the same as
"toByteStream" in HTTP? Use the same name?

https://codereview.chromium.org/11830017/diff/1/utils/pub/io.dart#newcode568
utils/pub/io.dart:568: if (stream.closed) return new
StreamController.singleSubscription()..close();
The duplicate constructor is a bit gross. How about just:

var controller = new StreamController.singleSubscription();

if (stream.closed) return controller..close();

stream.onClosed = ...
...

https://codereview.chromium.org/11830017/diff/1/utils/pub/io.dart#newcode577
utils/pub/io.dart:577: // TODO(nweiz): remove this ASAP
Tracking bug?

https://codereview.chromium.org/11830017/diff/1/utils/pub/io.dart#newcode579
utils/pub/io.dart:579: InputStream wrapByteStream(Stream<List<int>> stream) {
Maybe these method names should be more explicit. How about "toInputStream".

https://codereview.chromium.org/11830017/diff/1/utils/pub/io.dart#newcode588
utils/pub/io.dart:588: StreamConsumer<List<int>, dynamic>
wrapOutputStream(OutputStream stream) =>
"toStreamConsumer" ?

https://codereview.chromium.org/11830017/diff/1/utils/pub/io.dart#newcode596
utils/pub/io.dart:596: : super();
Unnecessary, especially since you aren't extending StreamConsumer. Even if you
were, I think it will do this implicitly if you omit it.

https://codereview.chromium.org/11830017/diff/1/utils/pub/io.dart#newcode614
utils/pub/io.dart:614: if (!completed) completer.completeError(e);
Want to do that little hack to grab a stack trace for this?

https://codereview.chromium.org/11830017/diff/1/utils/pub/io.dart#newcode682
utils/pub/io.dart:682: /// input stream. This is useful for spawned processes
which will not exit until
Long line.

https://codereview.chromium.org/11830017/diff/1/utils/pub/utils.dart
File utils/pub/utils.dart (left):

https://codereview.chromium.org/11830017/diff/1/utils/pub/utils.dart#oldcode48
utils/pub/utils.dart:48: 
Yay!

https://codereview.chromium.org/11830017/diff/1/utils/pub/utils.dart
File utils/pub/utils.dart (right):

https://codereview.chromium.org/11830017/diff/1/utils/pub/utils.dart#newcode125
utils/pub/utils.dart:125: /// The returned [Future] always has the same value as
[future].
Since this is how then() and catchError() work, it seems like this should be the
default behavior for whenComplete() too. File a bug?

https://codereview.chromium.org/11830017/diff/1/utils/pub/utils.dart#newcode211
utils/pub/utils.dart:211: // TODO(rnystrom): Remove this when #7781 is fixed.
"nweiz", not that it really matters. :)