Create a "PackageRef" class that defines a versionless package reference.

Create a "PackageRef" class that defines a versionless package reference.

R=nweiz@google.com

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

[nweiz, 2013-04-25 21:07:19]

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/package.dart (left):

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:138: int compareTo(PackageId other) {
What were we using this for?

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:81: /// which precisely identifies a
single concrete package version in the world
I think "in the world of packages" is more confusing than it's worth here.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:82: /// of packages, and [PackageDep]
which identifies one package's dependency on
The name "PackageDep" is fine and all, but I think we should document it in
terms of its structure rather than its use. So for example, instead of saying
"which identifies one package's dependency on another", say "which identifies a
range of versions of a package."

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:84: /// package.
"Conceptually identified" is a confusing term that isn't defined anywhere. I'd
rather be more explicit and say "a reference to a package, but not any specific
version(s) of that package".

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:115: // and that we don't need to
delve into the description.
I think this should still be a TODO. "git://github.com/nex3/foo.git" shouldn't
be == to "git://github.com/munificent/foo.git".

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:128: PackageRef toRef() => new
PackageRef(name, source, description);
This method seems weird, since every subclass should behave as a PackageRef
anyway. But they won't behave that way with == or hashCode, so I guess it's
fine.

Basically I just don't like the intersection of equality and inheritance.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:133: new PackageId(name, source,
version, description);
Should we have one of these to create a PackageDep, for good measure?

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:141: /// this reference.
Is this useful for anything other than PackageDep?

If so, explain the semantics of this for each class in their respective
docstrings.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:153: /// An unambiguous resolved
reference to a package. A package ID contains enough
I'd describe this as "A reference to a specific version of a package."

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:157: /// different directories that
happen to contain identical packages. For
"Directories" is a little inaccurate here; I would say "different packages that
have identical contents".

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:160: class PackageId extends
PackageRef {
The inheritance structure of these classes is weird. Why is PackageId a
PackageRef, but not a PackageDep? Do we get any value from being able to treat
PackageId and PackageDep as a PackageRef (other than making the implementations
more concise)? Does this value outweigh the risk of passing a PackageId to a
method that's annotated as PackageRef but expects a concrete PackageRef?

My preference would be to remove all inheritance relationships here (or add a
private base class so they're invisible). I think it confuses the semantics more
than it adds value. We can add methods like toRef/atVersion to convert between
them explicitly.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:178: return other.name == name &&
Add the above TODO about description checks here as well.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:198: return new PackageDep(name,
source, version, description);
Woah, this is very confusing. I expected toRef to always return a PackageRef,
not a subclass. I thought that was the whole point. Now for PackageId it returns
a PackageDep instead? What's going on here?

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:200: }
If you keep getVersions in PackageRef, this should override it.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:202: /// A dependency on a package.
Unlike a [PackageId], a PackageDep may not
See above about using a structure- rather than usage-based description.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:221: return other.name == name &&
Add the TODO message here too.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:225: }
This should definitely override getVersions.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:542:
List<Dependency> _getDependencies(String name) {
We're always passing `dep.name` here. Maybe this should just take a PackageRef
and extract the name manually.

[Bob Nystrom, 2013-04-26 22:17:49]

Broke the subclassing between PackageRef and PackageID and PackageDep. Much
better now.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/package.dart (left):

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:138: int compareTo(PackageId other) {
On 2013/04/25 21:07:19, nweiz wrote:
> What were we using this for?

I think the old solver may have used it? I'm not really sure. Nothing broke when
I took it out, so I think it was just dead code.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:81: /// which precisely identifies a
single concrete package version in the world
On 2013/04/25 21:07:19, nweiz wrote:
> I think "in the world of packages" is more confusing than it's worth here.

Done.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:82: /// of packages, and [PackageDep]
which identifies one package's dependency on
On 2013/04/25 21:07:19, nweiz wrote:
> The name "PackageDep" is fine and all, but I think we should document it in
> terms of its structure rather than its use. So for example, instead of saying
> "which identifies one package's dependency on another", say "which identifies
a
> range of versions of a package."

Done.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:84: /// package.
On 2013/04/25 21:07:19, nweiz wrote:
> "Conceptually identified" is a confusing term that isn't defined anywhere. I'd
> rather be more explicit and say "a reference to a package, but not any
specific
> version(s) of that package".

Done.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:115: // and that we don't need to
delve into the description.
On 2013/04/25 21:07:19, nweiz wrote:
> I think this should still be a TODO. "git://github.com/nex3/foo.git" shouldn't
> be == to "git://github.com/munificent/foo.git".

Done.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:128: PackageRef toRef() => new
PackageRef(name, source, description);
On 2013/04/25 21:07:19, nweiz wrote:
> This method seems weird, since every subclass should behave as a PackageRef
> anyway. But they won't behave that way with == or hashCode, so I guess it's
> fine.
> 
> Basically I just don't like the intersection of equality and inheritance.

This is less weird now that PackageRef isn't the base class of PackageID and
PackageDep.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:133: new PackageId(name, source,
version, description);
On 2013/04/25 21:07:19, nweiz wrote:
> Should we have one of these to create a PackageDep, for good measure?

We don't have a use case for that yet, so in the absence of that, I'd rather not
add dead code. It's easy to add once we need it.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:141: /// this reference.
On 2013/04/25 21:07:19, nweiz wrote:
> Is this useful for anything other than PackageDep?
> 
> If so, explain the semantics of this for each class in their respective
> docstrings.

It's actually only called right now given an instance of PackageRef, not
PackageDep. This method is a little strange in general. Because PackageRef has a
reference to the actual Source object, it has enough state to be able to
implement this, so I moved it here. It seemed less strange then doing:

someRef.source.getVersions(someRef);

It's weird to call a method on a field of an object and pass that selfsame
object back in as a paramater.

Part of this strangeness is that PackageRef (and its subclasses) are
conceptually lightweight identifiers but by having the actual Source object,
they can actually do real work.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:153: /// An unambiguous resolved
reference to a package. A package ID contains enough
On 2013/04/25 21:07:19, nweiz wrote:
> I'd describe this as "A reference to a specific version of a package."

Done.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:157: /// different directories that
happen to contain identical packages. For
On 2013/04/25 21:07:19, nweiz wrote:
> "Directories" is a little inaccurate here; I would say "different packages
that
> have identical contents".

Done.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:160: class PackageId extends
PackageRef {
On 2013/04/25 21:07:19, nweiz wrote:
> The inheritance structure of these classes is weird. Why is PackageId a
> PackageRef, but not a PackageDep? Do we get any value from being able to treat
> PackageId and PackageDep as a PackageRef (other than making the
implementations
> more concise)?

No, it's pure code reuse.

> Does this value outweigh the risk of passing a PackageId to a
> method that's annotated as PackageRef but expects a concrete PackageRef?
> 
> My preference would be to remove all inheritance relationships here (or add a
> private base class so they're invisible). I think it confuses the semantics
more
> than it adds value. We can add methods like toRef/atVersion to convert between
> them explicitly.

Done.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:178: return other.name == name &&
On 2013/04/25 21:07:19, nweiz wrote:
> Add the above TODO about description checks here as well.

Done.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:198: return new PackageDep(name,
source, version, description);
On 2013/04/25 21:07:19, nweiz wrote:
> Woah, this is very confusing. I expected toRef to always return a PackageRef,
> not a subclass. I thought that was the whole point. Now for PackageId it
returns
> a PackageDep instead? What's going on here?

Haha, yes! Very confusing!

This is an old dead method whose name toRef() referred to the *old* name before
I renamed PackageRef -> PackageDep. Removed. Oops!

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:200: }
On 2013/04/25 21:07:19, nweiz wrote:
> If you keep getVersions in PackageRef, this should override it.

It's in PackageRef, but PackageRef is no longer a base class for PackageDep.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:202: /// A dependency on a package.
Unlike a [PackageId], a PackageDep may not
On 2013/04/25 21:07:19, nweiz wrote:
> See above about using a structure- rather than usage-based description.

Done.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:221: return other.name == name &&
On 2013/04/25 21:07:19, nweiz wrote:
> Add the TODO message here too.

Done.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/package.dart:225: }
On 2013/04/25 21:07:19, nweiz wrote:
> This should definitely override getVersions.

This no longer inherits from a type that defines that.

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/13952013/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:542:
List<Dependency> _getDependencies(String name) {
On 2013/04/25 21:07:19, nweiz wrote:
> We're always passing `dep.name` here. Maybe this should just take a PackageRef
> and extract the name manually.

_getRequired() calls it with just a string.

[nweiz, 2013-04-27 00:12:18]

lgtm!

I like this change a lot post-inheritance, it makes it a lot clearer when the
code is referring to which sort of package.

https://codereview.chromium.org/13952013/diff/5001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/13952013/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/package.dart:135: if (other is! PackageRef) return
false;
You can write this as "return other is PackageRef && ...", also below

[Bob Nystrom, 2013-04-30 17:40:28]

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

[Bob Nystrom, 2013-04-30 17:44:25]

https://codereview.chromium.org/13952013/diff/5001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/13952013/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/package.dart:135: if (other is! PackageRef) return
false;
On 2013/04/27 00:12:18, nweiz wrote:
> You can write this as "return other is PackageRef && ...", also below

Done.