Get codebase ready for actually supporting versioning.

Get codebase ready for actually supporting versioning.

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

[Bob Nystrom, 2012-05-17 22:35:31]

This patch doesn't do anything visibly interesting, but it does reorganize
things to get ready for actually depending on versioned packages and being able
to resolve those dependencies.

[nweiz, 2012-05-18 00:08:24]

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/git_sourc...
File utils/pub/git_source.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/git_sourc...
utils/pub/git_source.dart:26: var url = id.description;
This should at least validate that url is a String.

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/package.dart
File utils/pub/package.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/package.d...
utils/pub/package.dart:55: Package._(String dir, SystemCache cache,
this.pubspec)
I'm not sure I like Package having a reference to the SystemCache, and
especially not to the PackagesDir. I think we need to figure out exactly what
the Package class represents.

I've been thinking of Package as a thin data class similar to PackageId and now
PackageRef. My conceptual model is that you start with a PackageRef, resolve it
to get a PackageId, and install+load that to get a Package. The actual
resolution, installation, and loading would be handled externally to the Package
class (although the loading may be part of a factory constructor).

I suggest using PackagesDir#installTransitively directly rather than
Package#installDependencies, and moving Package#resolve to PackagesDir. Once we
have a lockfile, that seems like it should be managed by PackagesDir as well.

It's possible PackagesDir should be renamed to EntrypointCache or something like
that.

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/package.d...
utils/pub/package.dart:158: class PackageRef {
What do you think about PackageRange or PackageConstraint as a name for this? I
feel like "Ref" isn't very descriptive.

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/pubspec.dart
File utils/pub/pubspec.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/pubspec.d...
utils/pub/pubspec.dart:66: // TODO(nweiz): parse the version once we have
version handling
You can remove this now

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/pubspec.d...
utils/pub/pubspec.dart:67: if (spec == null || spec is String) {
If spec is a string, you should parse it as a version.

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/pubspec.d...
utils/pub/pubspec.dart:74: version = new Version.parse(spec['version']);
Replace spec['version'] here with spec.remove('version')

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/pubspec.d...
utils/pub/pubspec.dart:112: * the pubspec.
I wouldn't document the default here when it only acquires that default if you
use Pubspec.parse. That actually probably belongs in the prose documentation.

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/sdk_sourc...
File utils/pub/sdk_source.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/sdk_sourc...
utils/pub/sdk_source.dart:28: var sourcePath = join(rootDir, "lib",
id.description);
This should validate that id.description is a string, as should packageName.
Maybe Source should have a validateDescription method that's automatically
called before calling packageName or install?

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/source.dart
File utils/pub/source.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/source.da...
utils/pub/source.dart:51: abstract String packageName(PackageId id);
I feel like id.description is still a good default for this.

[Bob Nystrom, 2012-05-18 20:02:38]

I think I responded to everything. Thanks!

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/git_sourc...
File utils/pub/git_source.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/git_sourc...
utils/pub/git_source.dart:26: var url = id.description;
On 2012/05/18 00:08:24, nweiz wrote:
> This should at least validate that url is a String.

Done. The GitSource validates the description when the pubspec is parsed so it
should be safe after that.

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/package.dart
File utils/pub/package.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/package.d...
utils/pub/package.dart:55: Package._(String dir, SystemCache cache,
this.pubspec)
On 2012/05/18 00:08:24, nweiz wrote:
> I'm not sure I like Package having a reference to the SystemCache, and
> especially not to the PackagesDir. I think we need to figure out exactly what
> the Package class represents.
> 
> I've been thinking of Package as a thin data class similar to PackageId and
now
> PackageRef. My conceptual model is that you start with a PackageRef, resolve
it
> to get a PackageId, and install+load that to get a Package. The actual
> resolution, installation, and loading would be handled externally to the
Package
> class (although the loading may be part of a factory constructor).
> 
> I suggest using PackagesDir#installTransitively directly rather than
> Package#installDependencies, and moving Package#resolve to PackagesDir. Once
we
> have a lockfile, that seems like it should be managed by PackagesDir as well.
> 
> It's possible PackagesDir should be renamed to EntrypointCache or something
like
> that.

Done.

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/package.d...
utils/pub/package.dart:158: class PackageRef {
On 2012/05/18 00:08:24, nweiz wrote:
> What do you think about PackageRange or PackageConstraint as a name for this?
I
> feel like "Ref" isn't very descriptive.

I <3 Ref as per our discussion.

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/pubspec.dart
File utils/pub/pubspec.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/pubspec.d...
utils/pub/pubspec.dart:66: // TODO(nweiz): parse the version once we have
version handling
On 2012/05/18 00:08:24, nweiz wrote:
> You can remove this now

Done.

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/pubspec.d...
utils/pub/pubspec.dart:67: if (spec == null || spec is String) {
On 2012/05/18 00:08:24, nweiz wrote:
> If spec is a string, you should parse it as a version.

Done.

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/pubspec.d...
utils/pub/pubspec.dart:74: version = new Version.parse(spec['version']);
On 2012/05/18 00:08:24, nweiz wrote:
> Replace spec['version'] here with spec.remove('version')

Done.

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/pubspec.d...
utils/pub/pubspec.dart:112: * the pubspec.
On 2012/05/18 00:08:24, nweiz wrote:
> I wouldn't document the default here when it only acquires that default if you
> use Pubspec.parse. That actually probably belongs in the prose documentation.

Done.

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/sdk_sourc...
File utils/pub/sdk_source.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/sdk_sourc...
utils/pub/sdk_source.dart:28: var sourcePath = join(rootDir, "lib",
id.description);
On 2012/05/18 00:08:24, nweiz wrote:
> This should validate that id.description is a string, as should packageName.
> Maybe Source should have a validateDescription method that's automatically
> called before calling packageName or install?

Good idea. Done.

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/source.dart
File utils/pub/source.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/2001/utils/pub/source.da...
utils/pub/source.dart:51: abstract String packageName(PackageId id);
On 2012/05/18 00:08:24, nweiz wrote:
> I feel like id.description is still a good default for this.

Done.

[nweiz, 2012-05-18 21:16:01]

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/entrypoin...
File utils/pub/entrypoint.dart (left):

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/entrypoin...
utils/pub/entrypoint.dart:42: // TODO(rnystrom): Make this path configurable.
This TODO should probably come back now, right?

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/entrypoin...
File utils/pub/entrypoint.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/entrypoin...
utils/pub/entrypoint.dart:7: * "entrypoint" package. This is typically the
package where the current
I'd make it more explicit that an entrypoint could be an application or a
library.

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/entrypoin...
utils/pub/entrypoint.dart:9: * associated with and is responsible for building
the "packages" directory
s/building/managing/

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/entrypoin...
utils/pub/entrypoint.dart:98: Future installTransitively(Package entrypoint,
Taking "Package entrypoint" here seems weird for a couple reasons. First, isn't
it always going to be identical to this.root? Second, "entrypoint" is a
confusing name for a package when it's also the name of this class.

Given that we're starting to think of installing dependencies as a holistic
action that involves resolving the whole dependency tree, it might make sense to
remove this method entirely and just move its contents into
#installDependencies.

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/package.dart
File utils/pub/package.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/package.d...
utils/pub/package.dart:70: Future<PackageId> resolve(PackageRef ref) {
I don't think resolve belongs in Package either. We're going to want this to
have access to all the dependency information in Entrypoint.

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/pubspec.dart
File utils/pub/pubspec.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/pubspec.d...
utils/pub/pubspec.dart:82: 'Dependency $name may not have multiple sources:
$sourceNames.');
Line length

[Bob Nystrom, 2012-05-18 22:13:04]

Thanks!

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/entrypoin...
File utils/pub/entrypoint.dart (left):

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/entrypoin...
utils/pub/entrypoint.dart:42: // TODO(rnystrom): Make this path configurable.
On 2012/05/18 21:16:01, nweiz wrote:
> This TODO should probably come back now, right?

Done.

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/entrypoin...
File utils/pub/entrypoint.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/entrypoin...
utils/pub/entrypoint.dart:7: * "entrypoint" package. This is typically the
package where the current
On 2012/05/18 21:16:01, nweiz wrote:
> I'd make it more explicit that an entrypoint could be an application or a
> library.

Done.

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/entrypoin...
utils/pub/entrypoint.dart:9: * associated with and is responsible for building
the "packages" directory
On 2012/05/18 21:16:01, nweiz wrote:
> s/building/managing/

Done.

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/entrypoin...
utils/pub/entrypoint.dart:98: Future installTransitively(Package entrypoint,
On 2012/05/18 21:16:01, nweiz wrote:
> Taking "Package entrypoint" here seems weird for a couple reasons. First,
isn't
> it always going to be identical to this.root?

Yeah, that's just stale code from before this refactoring.

> Second, "entrypoint" is a
> confusing name for a package when it's also the name of this class.

Ditto here.

> Given that we're starting to think of installing dependencies as a holistic
> action that involves resolving the whole dependency tree, it might make sense
to
> remove this method entirely and just move its contents into
> #installDependencies.

Good call. Done.

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/package.dart
File utils/pub/package.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/package.d...
utils/pub/package.dart:70: Future<PackageId> resolve(PackageRef ref) {
On 2012/05/18 21:16:01, nweiz wrote:
> I don't think resolve belongs in Package either. We're going to want this to
> have access to all the dependency information in Entrypoint.

Exactly right. Done.

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/pubspec.dart
File utils/pub/pubspec.dart (right):

https://chromiumcodereview.appspot.com/10399076/diff/8001/utils/pub/pubspec.d...
utils/pub/pubspec.dart:82: 'Dependency $name may not have multiple sources:
$sourceNames.');
On 2012/05/18 21:16:01, nweiz wrote:
> Line length

Done.

[nweiz, 2012-05-18 22:21:51]

LGTM++