Add a Pub source that checks packages out from Git.

Add a Pub source that checks packages out from Git.

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

[Bob Nystrom, 2012-05-08 17:01:49]

A couple of minor nits but the code is fine. I really want to see tests and
ideally robust error-handling here too. We aren't in prototype mode, so I want
the parts of pub that are implemented to be solid.

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/git_source.dart
File utils/pub/git_source.dart (right):

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/git_source.d...
utils/pub/git_source.dart:8: class GitSource extends Source {
Where are the tests for this?

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/git_source.d...
utils/pub/git_source.dart:32: basename(id.fullName).replaceFirst(const
RegExp("\.git\$"), "");
Either make this a {} body (which I would prefer, but I know you won't :) ) or
at least indent it +4 since it's a line continuation.

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/io.dart#newc...
utils/pub/io.dart:325: * output stream(s) to the parent process's output
stream(s). Output from piped
"to the" -> "are sent to the".
Also "(s)" -> "s".

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/pub.dart
File utils/pub/pub.dart (right):

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/pub.dart#new...
utils/pub/pub.dart:75: // TODO(rnystrom): Do we want this to be global?
If you don't mind, remove this TODO. I think we've decided we don't. :)

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/source_regis...
File utils/pub/source_registry.dart (right):

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/source_regis...
utils/pub/source_registry.dart:15: SourceRegistry() : _map = new Map<String,
Source>();
<Source>{}

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/source_regis...
utils/pub/source_registry.dart:28: // TODO(nweiz): Real error-handling system
Now may be as good a time as any to do the right error-handling here. I worry
about building up too much technical debt around robustness and ended up with a
brittle pub that doesn't make people happy relying on it.

[nweiz, 2012-05-09 00:21:04]

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/git_source.dart
File utils/pub/git_source.dart (right):

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/git_source.d...
utils/pub/git_source.dart:8: class GitSource extends Source {
On 2012/05/08 17:01:49, Bob Nystrom wrote:
> Where are the tests for this?

Just confirmed with Emily that we can run Git on the test bots, so I'll write
some.

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/git_source.d...
utils/pub/git_source.dart:32: basename(id.fullName).replaceFirst(const
RegExp("\.git\$"), "");
On 2012/05/08 17:01:49, Bob Nystrom wrote:
> Either make this a {} body (which I would prefer, but I know you won't :) ) or
> at least indent it +4 since it's a line continuation.

Done.

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/io.dart#newc...
utils/pub/io.dart:325: * output stream(s) to the parent process's output
stream(s). Output from piped
On 2012/05/08 17:01:49, Bob Nystrom wrote:
> "to the" -> "are sent to the".
> Also "(s)" -> "s".

Done.

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/pub.dart
File utils/pub/pub.dart (right):

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/pub.dart#new...
utils/pub/pub.dart:75: // TODO(rnystrom): Do we want this to be global?
On 2012/05/08 17:01:49, Bob Nystrom wrote:
> If you don't mind, remove this TODO. I think we've decided we don't. :)

Done.

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/source_regis...
File utils/pub/source_registry.dart (right):

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/source_regis...
utils/pub/source_registry.dart:15: SourceRegistry() : _map = new Map<String,
Source>();
On 2012/05/08 17:01:49, Bob Nystrom wrote:
> <Source>{}

Done.

https://chromiumcodereview.appspot.com/10386021/diff/1/utils/pub/source_regis...
utils/pub/source_registry.dart:28: // TODO(nweiz): Real error-handling system
On 2012/05/08 17:01:49, Bob Nystrom wrote:
> Now may be as good a time as any to do the right error-handling here. I worry
> about building up too much technical debt around robustness and ended up with
a
> brittle pub that doesn't make people happy relying on it.

I'm going to do this in a separate CL, since it's pretty orthogonal, but I'll
make sure to get to it soon.

[Bob Nystrom, 2012-05-14 19:40:02]

LGTM.

https://chromiumcodereview.appspot.com/10386021/diff/1011/utils/tests/pub/pub...
File utils/tests/pub/pub_test.dart (right):

https://chromiumcodereview.appspot.com/10386021/diff/1011/utils/tests/pub/pub...
utils/tests/pub/pub_test.dart:142: String _gitPubspec(String name) => '''
As much as I like DRY, I think the tests would be easier to read if they were
just literal strings for the pubspecs.

https://chromiumcodereview.appspot.com/10386021/diff/1011/utils/tests/pub/pub...
utils/tests/pub/pub_test.dart:164: output: const RegExp("^Cloning
into[\\s\\S]*^Dependencies installed!\$",
You can use a raw string instead of escaping \:

@"^Cloning into[\s\S]*..."

You may still have to escape the $, though. Not sure.

[nweiz, 2012-05-14 19:53:38]

https://chromiumcodereview.appspot.com/10386021/diff/1011/utils/tests/pub/pub...
File utils/tests/pub/pub_test.dart (right):

https://chromiumcodereview.appspot.com/10386021/diff/1011/utils/tests/pub/pub...
utils/tests/pub/pub_test.dart:142: String _gitPubspec(String name) => '''
On 2012/05/14 19:40:02, Bob Nystrom wrote:
> As much as I like DRY, I think the tests would be easier to read if they were
> just literal strings for the pubspecs.

Done.

https://chromiumcodereview.appspot.com/10386021/diff/1011/utils/tests/pub/pub...
utils/tests/pub/pub_test.dart:164: output: const RegExp("^Cloning
into[\\s\\S]*^Dependencies installed!\$",
On 2012/05/14 19:40:02, Bob Nystrom wrote:
> You can use a raw string instead of escaping \:
> 
> @"^Cloning into[\s\S]*..."
> 
> You may still have to escape the $, though. Not sure.

Done.