Add "pub global list" command to show all globally activated packages.

Add "pub global list" command to show all globally activated packages.
Clean up global_packages.dart.

BUG=http://dartbug.com/19962
R=rnystrom@google.com

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

[Anton Moiseev, 2014-08-20 13:49:35]

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

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:171: log.message('Activated
${_formatPackage(id)}.');
Didn't want to duplicate this the fourth time, moved "if" block to the
_formatPackage() method. However, due to a little bit different format log
message for the deactivate command has been changed - for Git and Path sources
it now always displays package version:

before: Deactivated package foo from Git repository "../foo.git".
after:  Deactivated package foo 1.0.0 from Git repository "../foo.git".

While users might be not interested in particular version number when
deactivating a package, I think it doesn't hurt and benefit from avoiding
duplication is more appealing :). But if you want me to roll this back, please
let me know.

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:178: void
_describeActive(String name) {
No meaningful changes here. Noticed that actual parameter's name doesn't match
the name in the doc comment and renamed `package` to `name` to align naming with
other usages within the class. In turn all the lines were re-wrapped in order to
conform the style guide.

[Bob Nystrom, 2014-08-20 17:21:22]

This is really nice, thank you!

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

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:171: log.message('Activated
${_formatPackage(id)}.');
On 2014/08/20 13:49:34, Anton Moiseev wrote:
> Didn't want to duplicate this the fourth time, moved "if" block to the
> _formatPackage() method. 

Excellent, I was hoping you'd do that. :)

> However, due to a little bit different format log
> message for the deactivate command has been changed - for Git and Path sources
> it now always displays package version:
> 
> before: Deactivated package foo from Git repository "../foo.git".
> after:  Deactivated package foo 1.0.0 from Git repository "../foo.git".
> 
> While users might be not interested in particular version number when
> deactivating a package, I think it doesn't hurt and benefit from avoiding
> duplication is more appealing :). But if you want me to roll this back, please
> let me know.

Putting the version in here sounds good to me too.

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:178: void
_describeActive(String name) {
On 2014/08/20 13:49:35, Anton Moiseev wrote:
> No meaningful changes here. Noticed that actual parameter's name doesn't match
> the name in the doc comment and renamed `package` to `name` to align naming
with
> other usages within the class. In turn all the lines were re-wrapped in order
to
> conform the style guide.

Thanks!

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:268: List<PackageId>
getActivePackages({bool logPackages: false}) {
Right now, since this is only used by pub global list, I think it's simpler to
just make this:

void listActivePackages()

and have it always log.

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:271: return
listDir(_directory).map((file) {
Skip files that don't end with .lock. Otherwise, you will get bogus stuff like
".DS_Store", etc.

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:274: var packageId =
lockFile.packages[name];
Nit, but "packageId" -> "id". I think we tend to use "id" most commonly in this
library.

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:281: }).toList();
We should sort the results alphabetically by package name before logging them.
Some OSes seem to give sorted results when you list a directory, but it isn't
guaranteed.

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/test/g...
File sdk/lib/_internal/pub/test/global/list_test.dart (right):

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/list_test.dart:57: });
These tests are great. :)

One more: activate a couple of packages and verify that they're listed in
alphabetical order.

[Anton Moiseev, 2014-08-20 21:22:47]

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

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:171: log.message('Activated
${_formatPackage(id)}.');
On 2014/08/20 17:21:21, Bob Nystrom wrote:
> On 2014/08/20 13:49:34, Anton Moiseev wrote:
> > Didn't want to duplicate this the fourth time, moved "if" block to the
> > _formatPackage() method. 
> 
> Excellent, I was hoping you'd do that. :)
> 
> > However, due to a little bit different format log
> > message for the deactivate command has been changed - for Git and Path
sources
> > it now always displays package version:
> > 
> > before: Deactivated package foo from Git repository "../foo.git".
> > after:  Deactivated package foo 1.0.0 from Git repository "../foo.git".
> > 
> > While users might be not interested in particular version number when
> > deactivating a package, I think it doesn't hurt and benefit from avoiding
> > duplication is more appealing :). But if you want me to roll this back,
please
> > let me know.
> 
> Putting the version in here sounds good to me too.

Acknowledged.

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:268: List<PackageId>
getActivePackages({bool logPackages: false}) {
On 2014/08/20 17:21:21, Bob Nystrom wrote:
> Right now, since this is only used by pub global list, I think it's simpler to
> just make this:
> 
> void listActivePackages()
> 
> and have it always log.

Makes sense. Done.

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:271: return
listDir(_directory).map((file) {
On 2014/08/20 17:21:22, Bob Nystrom wrote:
> Skip files that don't end with .lock. Otherwise, you will get bogus stuff like
> ".DS_Store", etc.

I was assuming hidden files are skipped by default, but hadn't added a guard for
other type of files. Fixed, thanks!

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:274: var packageId =
lockFile.packages[name];
On 2014/08/20 17:21:22, Bob Nystrom wrote:
> Nit, but "packageId" -> "id". I think we tend to use "id" most commonly in
this
> library.

Right, thanks.

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:281: }).toList();
On 2014/08/20 17:21:21, Bob Nystrom wrote:
> We should sort the results alphabetically by package name before logging them.
> Some OSes seem to give sorted results when you list a directory, but it isn't
> guaranteed.

Good point, fixed, thank you.

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/test/g...
File sdk/lib/_internal/pub/test/global/list_test.dart (right):

https://codereview.chromium.org/489823002/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/list_test.dart:57: });
On 2014/08/20 17:21:22, Bob Nystrom wrote:
> These tests are great. :)
> 
> One more: activate a couple of packages and verify that they're listed in
> alphabetical order.

Absolutely! Thanks!

[Bob Nystrom, 2014-08-20 21:35:09]

https://codereview.chromium.org/489823002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/global_packages.dart (right):

https://codereview.chromium.org/489823002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:267: /// Loads lock [file]
and returns [PackageId] of the activated package.
We don't generally use doc comments for local functions (since the doc generator
doesn't seem them anyway), but the comment here is fine. Just change /// to //.
You can remove the type annotations too. Our philosophy is that if a local
function is big enough for the type annotations to be needed, it's probably big
enough to move it out into its own top-level function/method.

You could just make this a lambda in the call to map() too if you like.

https://codereview.chromium.org/489823002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:276: .map((file) =>
loadPackageId(file))
.map(loadPackageId)

https://codereview.chromium.org/489823002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:278: ..sort((id1, id2) =>
id1.name.compareTo(id2.name))
This is personal preference, but I find ".." hard to read when mixed with a
multi-line statement also using ".". How about:

var packages = listDir(...)
    .where(...)
    .map(...)
    .toList();
packages
    ..sort()
    ..forEach(...);

[Anton Moiseev, 2014-08-20 22:09:05]

https://codereview.chromium.org/489823002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/global_packages.dart (right):

https://codereview.chromium.org/489823002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:267: /// Loads lock [file]
and returns [PackageId] of the activated package.
On 2014/08/20 21:35:09, Bob Nystrom wrote:
> We don't generally use doc comments for local functions (since the doc
generator
> doesn't seem them anyway), but the comment here is fine. Just change /// to
//.
> You can remove the type annotations too. Our philosophy is that if a local
> function is big enough for the type annotations to be needed, it's probably
big
> enough to move it out into its own top-level function/method.
> 
> You could just make this a lambda in the call to map() too if you like.

Understood.

https://codereview.chromium.org/489823002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:276: .map((file) =>
loadPackageId(file))
On 2014/08/20 21:35:09, Bob Nystrom wrote:
> .map(loadPackageId)

Ah... Thanks.

https://codereview.chromium.org/489823002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:278: ..sort((id1, id2) =>
id1.name.compareTo(id2.name))
On 2014/08/20 21:35:09, Bob Nystrom wrote:
> This is personal preference, but I find ".." hard to read when mixed with a
> multi-line statement also using ".". How about:
> 
> var packages = listDir(...)
>     .where(...)
>     .map(...)
>     .toList();
> packages
>     ..sort()
>     ..forEach(...);
> 

Totally agree. I personally find ".." quite distinguishable (probably too much),
but it's another extreme :) - just don't like how this long chain with mixed
approaches reads and looks like.

[Bob Nystrom, 2014-08-21 16:45:52]

LGTM, thank you!

I'll land this.

- bob

[Bob Nystrom, 2014-08-21 18:03:51]

Committed patchset #3 manually as 39466 (presubmit successful).

[Anton Moiseev, 2014-08-21 21:48:49]

On 2014/08/21 16:45:52, Bob Nystrom wrote:
> LGTM, thank you!
> 
> I'll land this.
> 
> - bob

Thanks for patiently addressing all the patches ;) and explaining not only how,
but also why something should be done!