[pub] clean up Editor messages

[pub] clean up Editor messages
* missing returns
* unused imports

R=rnystrom@google.com

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

[Jennifer Messerly, 2014-03-04 03:56:36]

Noticed a few warnings/infos/whatever-theyre-called while browsing Pub sources.
Thought I'd send a patch :)

[Bob Nystrom, 2014-03-04 17:08:44]

A couple of tweaks then LGTM. Thank you!

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

https://codereview.chromium.org/184823012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/server.dart:89: return null;
Oh, I've been meaning to fix this. It should really be:

return Future.wait([
  _server.close(),
  _resultsController.close()
]);

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

https://codereview.chromium.org/184823012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source.dart:295: return null;
This should:

throw new UnsupportedError("Cannot call getCachedPackages() on an uncached
source.");

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

https://codereview.chromium.org/184823012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/version.dart:366: return null;
This is unreachable. I could change the last case to default, but that's unclear
and asymmetric with the other cases. Maybe:

throw new AssertionError("Unreachable.");

?

[Jennifer Messerly, 2014-03-04 19:19:26]

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

https://codereview.chromium.org/184823012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/version.dart:366: return null;
On 2014/03/04 17:08:44, Bob Nystrom wrote:
> This is unreachable. I could change the last case to default, but that's
unclear
> and asymmetric with the other cases. Maybe:
> 
> throw new AssertionError("Unreachable.");
> 
> ?

sure. Per Lasse AssertionError is only intended for assert and so it doesn't
have a message. Would you prefer AssertionError without a message or something
else like StateError("Unreachable.")?

[Bob Nystrom, 2014-03-04 19:30:58]

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

https://codereview.chromium.org/184823012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/version.dart:366: return null;
On 2014/03/04 19:19:27, John Messerly wrote:
> On 2014/03/04 17:08:44, Bob Nystrom wrote:
> > This is unreachable. I could change the last case to default, but that's
> unclear
> > and asymmetric with the other cases. Maybe:
> > 
> > throw new AssertionError("Unreachable.");
> > 
> > ?
> 
> sure. Per Lasse AssertionError is only intended for assert and so it doesn't
> have a message. Would you prefer AssertionError without a message or something
> else like StateError("Unreachable.")?

StateError isn't right because it implies there may be times where that code can
be reached. AssertionError was the best thing I could come up with, and that
feels gross too. (That's part of why we haven't fixed this warning.)

Maybe just:

throw "Unreachable."?

[Jennifer Messerly, 2014-03-04 20:00:58]

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

https://codereview.chromium.org/184823012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/version.dart:366: return null;
On 2014/03/04 19:30:58, Bob Nystrom wrote:
> On 2014/03/04 19:19:27, John Messerly wrote:
> > On 2014/03/04 17:08:44, Bob Nystrom wrote:
> > > This is unreachable. I could change the last case to default, but that's
> > unclear
> > > and asymmetric with the other cases. Maybe:
> > > 
> > > throw new AssertionError("Unreachable.");
> > > 
> > > ?
> > 
> > sure. Per Lasse AssertionError is only intended for assert and so it doesn't
> > have a message. Would you prefer AssertionError without a message or
something
> > else like StateError("Unreachable.")?
> 
> StateError isn't right because it implies there may be times where that code
can
> be reached. AssertionError was the best thing I could come up with, and that
> feels gross too. (That's part of why we haven't fixed this warning.)
> 
> Maybe just:
> 
> throw "Unreachable."?

Sure, will do that.

But, FWIW, I think StateError('Unreachable') is totally fine for this case:
* if code actually cannot be reached, the error is irrelevant, no one will see
it.
* if the code can be reached, perhaps as a mistake in a future change, then
StateError is correct -- the program reached in invalid state.

[Jennifer Messerly, 2014-03-04 20:08:19]

By the way -- to get Pub opened in the editor I had to create a fake
pubspec.yaml, otherwise analyzer refused to recognize my "package:" imports,
even though I had a "packages" symlink in there.

Do you think we could add a fake pubspec.yaml to the root of Pub? Or is it an
Editor bug that it requires that, since Dart VM will recognize "packages/"
regardless of the existence of pubspec.yaml?

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

https://codereview.chromium.org/184823012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/server.dart:89: return null;
On 2014/03/04 17:08:44, Bob Nystrom wrote:
> Oh, I've been meaning to fix this. It should really be:
> 
> return Future.wait([
>   _server.close(),
>   _resultsController.close()
> ]);

Yeah, I thought that was strange, wasn't sure if I should fix :). Done!

For my future reference, does the Pub coding style allow something more like:

  /// Closes this server.
  Future close() => Future.wait([
    _server.close(),
    _resultsController.close()
  ]);

(that's probably how I would write it in my own code.)

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

https://codereview.chromium.org/184823012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source.dart:295: return null;
On 2014/03/04 17:08:44, Bob Nystrom wrote:
> This should:
> 
> throw new UnsupportedError("Cannot call getCachedPackages() on an uncached
> source.");

Done.

[Jennifer Messerly, 2014-03-04 20:32:20]

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

[Bob Nystrom, 2014-03-04 20:45:21]

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

https://codereview.chromium.org/184823012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/server.dart:89: return null;
On 2014/03/04 20:08:20, John Messerly wrote:
> On 2014/03/04 17:08:44, Bob Nystrom wrote:
> > Oh, I've been meaning to fix this. It should really be:
> > 
> > return Future.wait([
> >   _server.close(),
> >   _resultsController.close()
> > ]);
> 
> Yeah, I thought that was strange, wasn't sure if I should fix :). Done!
> 
> For my future reference, does the Pub coding style allow something more like:
> 
>   /// Closes this server.
>   Future close() => Future.wait([
>     _server.close(),
>     _resultsController.close()
>   ]);
> 
> (that's probably how I would write it in my own code.)

Hmm, that's a weird corner case, but I guess it's OK.

[Bob Nystrom, 2014-03-04 20:46:04]

On 2014/03/04 20:08:19, John Messerly wrote:
> By the way -- to get Pub opened in the editor I had to create a fake
> pubspec.yaml, otherwise analyzer refused to recognize my "package:" imports,
> even though I had a "packages" symlink in there.
> 
> Do you think we could add a fake pubspec.yaml to the root of Pub? Or is it an
> Editor bug that it requires that, since Dart VM will recognize "packages/"
> regardless of the existence of pubspec.yaml?

You shouldn't have to create a pubspec, you can just specify an explicit package
root, which is what I do.

- bob