Added .packages, packages/ discovery.

Added .packages, packages/ discovery.

Complete restructure of repository, splitting into several files.

R=pquitslund@google.com

Committed: 5f8ce7330c4ed0397334a1672814874520978f61

[Lasse Reichstein Nielsen, 2015-05-21 12:47:37]

lrn@google.com changed reviewers:
+ pquitslund@google.com

[Lasse Reichstein Nielsen, 2015-05-21 12:47:38]

No extra tests added, so most likely it doesn't actually work yet.

[pquitslund, 2015-05-21 18:14:23]

LGTM % nits.

Thanks!

https://codereview.chromium.org/1142363005/diff/1/lib/discovery.dart
File lib/discovery.dart (right):

https://codereview.chromium.org/1142363005/diff/1/lib/discovery.dart#newcode42
lib/discovery.dart:42: */
Nit: maybe prefer the `///` comment convention as per the style guide?  (Or be
consistent either way.)

https://codereview.chromium.org/1142363005/diff/1/lib/src/packages_impl.dart
File lib/src/packages_impl.dart (right):

https://codereview.chromium.org/1142363005/diff/1/lib/src/packages_impl.dart#...
lib/src/packages_impl.dart:26: }
Maybe:

Iterable<String> get packages => new Iterable<String>.empty();

https://codereview.chromium.org/1142363005/diff/1/lib/src/packages_impl.dart#...
lib/src/packages_impl.dart:31: abstract class PackagesBase implements Packages {
Nit: class level comment for PackageBase?

https://codereview.chromium.org/1142363005/diff/1/lib/src/packages_impl.dart#...
lib/src/packages_impl.dart:75: }
Maybe:

Iterable<String> get packages => _listPackageNames();

https://codereview.chromium.org/1142363005/diff/1/lib/src/util.dart
File lib/src/util.dart (right):

https://codereview.chromium.org/1142363005/diff/1/lib/src/util.dart#newcode4
lib/src/util.dart:4: library package_config._util;
Not sure if this is intentional.  Should it be package_config.util? 
Alternatively, should util be _util.dart?

https://codereview.chromium.org/1142363005/diff/1/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/1142363005/diff/1/pubspec.yaml#newcode13
pubspec.yaml:13: http: "any"
Nit: "any" => 'any' (to stay consistent).

More substantially, is `any` perhaps too inclusive?  Or do we really feel like
that's safe?

[Lasse Reichstein Nielsen, 2015-05-21 18:30:22]

https://codereview.chromium.org/1142363005/diff/1/lib/discovery.dart
File lib/discovery.dart (right):

https://codereview.chromium.org/1142363005/diff/1/lib/discovery.dart#newcode42
lib/discovery.dart:42: */
I'm *trying* to use the new /// convention, but I keep slipping back :)

https://codereview.chromium.org/1142363005/diff/1/lib/src/packages_impl.dart
File lib/src/packages_impl.dart (right):

https://codereview.chromium.org/1142363005/diff/1/lib/src/packages_impl.dart#...
lib/src/packages_impl.dart:26: }
I think it fits on a line, so yes.

https://codereview.chromium.org/1142363005/diff/1/lib/src/packages_impl.dart#...
lib/src/packages_impl.dart:31: abstract class PackagesBase implements Packages {
On 2015/05/21 18:14:23, pquitslund wrote:
> Nit: class level comment for PackageBase?

Acknowledged.

https://codereview.chromium.org/1142363005/diff/1/lib/src/packages_impl.dart#...
lib/src/packages_impl.dart:75: }
On 2015/05/21 18:14:22, pquitslund wrote:
> Maybe:
> 
> Iterable<String> get packages => _listPackageNames();

Acknowledged.

https://codereview.chromium.org/1142363005/diff/1/lib/src/util.dart
File lib/src/util.dart (right):

https://codereview.chromium.org/1142363005/diff/1/lib/src/util.dart#newcode4
lib/src/util.dart:4: library package_config._util;
Not sure what I was thinking. It should just be .util.

https://codereview.chromium.org/1142363005/diff/1/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/1142363005/diff/1/pubspec.yaml#newcode13
pubspec.yaml:13: http: "any"
TBH, I just put in "any" to get it to work. I have no idea what the available
versions are, and I hadn't gotten around to checking :)
I should just see which version I got in the lock file and request that.

[pquitslund, 2015-05-21 18:38:34]

https://codereview.chromium.org/1142363005/diff/1/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/1142363005/diff/1/pubspec.yaml#newcode13
pubspec.yaml:13: http: "any"
On 2015/05/21 18:30:22, Lasse Reichstein Nielsen wrote:
> TBH, I just put in "any" to get it to work. I have no idea what the available
> versions are, and I hadn't gotten around to checking :)
> I should just see which version I got in the lock file and request that.

:)  Or just leave it as any for now and we can tighten it moving forward.  The
trick is to make sure that whatever constraints we settle on play nice with what
we have in third_party/pkg.  Incidentally, I think we'll need to get charcode
into third_party before this can be used in the SDK.  Do you have plans to make
that repo public?

[Lasse Reichstein Nielsen, 2015-05-21 19:10:05]

https://codereview.chromium.org/1142363005/diff/1/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/1142363005/diff/1/pubspec.yaml#newcode13
pubspec.yaml:13: http: "any"
On 2015/05/21 18:38:33, pquitslund wrote:
> Incidentally, I think we'll need to get charcode
> into third_party before this can be used in the SDK.  Do you have plans to
make
> that repo public?

I didn't have any prior plan to do that, it's safely tugged away in an internal
repository, but I guess there's no hurt in making it public.

[pquitslund, 2015-05-21 20:25:29]

On 2015/05/21 19:10:05, Lasse Reichstein Nielsen wrote:
> https://codereview.chromium.org/1142363005/diff/1/pubspec.yaml
> File pubspec.yaml (right):
> 
> https://codereview.chromium.org/1142363005/diff/1/pubspec.yaml#newcode13
> pubspec.yaml:13: http: "any"
> On 2015/05/21 18:38:33, pquitslund wrote:
> > Incidentally, I think we'll need to get charcode
> > into third_party before this can be used in the SDK.  Do you have plans to
> make
> > that repo public?
> 
> I didn't have any prior plan to do that, it's safely tugged away in an
internal
> repository, but I guess there's no hurt in making it public.

Got it.  The rub is that in order to use package_config in the SDK all of its
dependencies need to be accounted for in third_party so that we can build from
source.  I think our options are 

1) make it public (and in dartlang) since it will be part of the SDK
2) remove the dependency (by inlining or otherwise)

Happy to help with whatever you land on.

[Lasse Reichstein Nielsen, 2015-05-22 10:43:36]

Committed patchset #2 (id:20001) manually as
5f8ce7330c4ed0397334a1672814874520978f61 (presubmit successful).

[Lasse Reichstein Nielsen, 2015-05-22 14:39:07]

https://codereview.chromium.org/1142363005/diff/1/lib/discovery.dart
File lib/discovery.dart (right):

https://codereview.chromium.org/1142363005/diff/1/lib/discovery.dart#newcode96
lib/discovery.dart:96: Packages findPackagesFromFile(Uri fileBaseUri) {
Added documentation.