Refactor Source and SourceRegistry.

lib/src/source.dart

Index: lib/src/source.dart
diff --git a/lib/src/source.dart b/lib/src/source.dart
index b2fb08397f8c4403b4adac3fb50b2c3d93d020ce..f077bba50dcc6a264d2ca24ca4b6129f401cd166 100644
--- a/lib/src/source.dart
+++ b/lib/src/source.dart
@@ -21,13 +21,20 @@ import 'utils.dart';
 /// package needs a dependency from a cached source, it is first installed in
 /// the [SystemCache] and then acquired from there.
 ///
+/// Each user-visible source has two classes: a [Source] that knows how to do
+/// filesystem-independent operations like parsing and comparing descriptions,
+/// and a [LiveSource] that knows how to actually install (and potentially
+/// download) those packages. Only the [LiveSource] has access to the
+/// [SystemCache].
+///
 /// ## Subclassing
 ///
-/// All sources should extend this class. In addition to defining the behavior
-/// of various methods, sources define the structure of package descriptions
-/// used in [PackageRef]s, [PackageDep]s, and [PackageId]s. There are three
-/// distinct types of description, although in practice most sources use the
-/// same format for one or more of these:
+/// All [Source]s should extend this class and all [LiveSource]s should extend
+/// [LiveSource]. In addition to defining the behavior of various methods,
+/// sources define the structure of package descriptions used in [PackageRef]s,
+/// [PackageDep]s, and [PackageId]s. There are three distinct types of
+/// description, although in practice most sources use the same format for one
+/// or more of these:
 ///
 /// * User descriptions. These are included in pubspecs and usually written by
 ///   hand. They're typically more flexible in the formats they allow to
@@ -55,32 +62,80 @@ abstract class Source {
   /// Defaults to `false`.
   final bool hasMultipleVersions = false;
 
-  /// Whether or not this source is the default source.
-  bool get isDefault => systemCache.sources.defaultSource == this;
+  /// Records the system cache to which this source belongs.
+  ///
+  /// This should only be called once for each source, by
+  /// [SystemCache.register]. It should not be overridden by base classes.
+  LiveSource bind(SystemCache systemCache);
 
-  /// A cache of pubspecs described by [describe].
-  final _pubspecs = <PackageId, Pubspec>{};
+  /// Parses a [PackageRef] from a name and a user-provided [description].
+  ///
+  /// When a [Pubspec] is parsed, it reads in the description for each
+  /// dependency. It is up to the dependency's [Source] to determine how that
+  /// should be interpreted. This will be called during parsing to validate that
+  /// the given [description] is well-formed according to this source, and to
+  /// give the source a chance to canonicalize the description.
+  ///
+  /// [containingPath] is the path to the local file (pubspec or lockfile)
+  /// where this description appears. It may be `null` if the description is
+  /// coming from some in-memory source (such as pulling down a pubspec from
+  /// pub.dartlang.org).
+  ///
+  /// The description in the returned [PackageRef] need bear no resemblance to
+  /// the original user-provided description.
+  ///
+  /// Throws a [FormatException] if the description is not valid.
+  PackageRef parseRef(String name, description, {String containingPath});
 
-  /// The system cache with which this source is registered.
-  SystemCache get systemCache {
-    assert(_systemCache != null);
-    return _systemCache;
-  }
+  /// Parses a [PackageId] from a name and a serialized description.
+  ///
+  /// This only accepts descriptions serialized using [serializeDescription]. It
+  /// should not be used with user-authored descriptions.
+  ///
+  /// Throws a [FormatException] if the description is not valid.
+  PackageId parseId(String name, Version version, description);
 
-  /// The system cache variable.
+  /// When a [LockFile] is serialized, it uses this method to get the
+  /// [description] in the right format.
   ///
-  /// Set by [_bind].
-  SystemCache _systemCache;
+  /// [containingPath] is the containing directory of the root package.
+  dynamic serializeDescription(String containingPath, description) {
+    return description;
+  }
 
-  /// Records the system cache to which this source belongs.
+  /// When a package [description] is shown to the user, this is called to
+  /// convert it into a human-friendly form.
   ///
-  /// This should only be called once for each source, by
-  /// [SystemCache.register]. It should not be overridden by base classes.
-  void bind(SystemCache systemCache) {
-    assert(_systemCache == null);
-    this._systemCache = systemCache;
+  /// By default, it just converts the description to a string, but sources
+  /// may customize this. [containingPath] is the containing directory of the
+  /// root package.
+  String formatDescription(String containingPath, description) {
+    return description.toString();
   }
 
+  /// Returns whether or not [description1] describes the same package as
+  /// [description2] for this source.
+  ///
+  /// This method should be light-weight. It doesn't need to validate that
+  /// either package exists.
+  ///
+  /// Note that either description may be a reference description or an ID
+  /// description; they need not be the same type. ID descriptions should be
+  /// considered equal to the reference descriptions that produced them.
+  bool descriptionsEqual(description1, description2);
+
+  /// Returns the source's name.
+  String toString() => name;
+}
+
+/// A source bound to a [SystemCache].
+abstract class LiveSource {

[Bob Nystrom, 2016/06/14 23:21:55]

"LiveSource" feels a little weird to me. It's sort of a mixed metaphor, since
"sources" aren't living things.

What do you think of "Provider"? Then we'd have "GitSource" and "GitProvider",
et al.

[nweiz, 2016/06/20 20:46:08]

"Provider" means more or less the same thing as "Source", which seems confusing.
What about "BoundSource"?

[Bob Nystrom, 2016/06/20 21:20:31]

Yeah, that bugged me too. I do feel they are a little different in that a
"provider" directly does the providing while a "source" is just something that
*could* provide. At least, that's what I told myself.
Meh. I feel like "Bound" is kind of a null adjective that gets used in a lot of
places to mean a lot of different things. Then you still have to do
"GitBoundSource", etc.

It's up to you either way.

[nweiz, 2016/06/20 22:03:18]

Going with BoundSource as discussed offline.

+  /// The unbound source that produced [this]. 
+  Source get source;
+
+  /// The system cache to which [this] is bound.
+  SystemCache get systemCache;
+
   /// Get the IDs of all versions that match [ref].
   ///
   /// Note that this does *not* require the packages to be downloaded locally,
@@ -96,8 +151,8 @@ abstract class Source {
     if (ref.isRoot) {
       throw new ArgumentError("Cannot get versions for the root package.");
     }
-    if (ref.source != name) {
-      throw new ArgumentError("Package $ref does not use source $name.");
+    if (ref.source != source.name) {
+      throw new ArgumentError("Package $ref does not use source ${source.name}.");
     }
 
     return doGetVersions(ref);
@@ -117,6 +172,9 @@ abstract class Source {
   /// external code should not call this. Instead, call [getVersions].
   Future<List<PackageId>> doGetVersions(PackageRef ref);
 
+  /// A cache of pubspecs described by [describe].
+  final _pubspecs = <PackageId, Pubspec>{};
+
   /// Loads the (possibly remote) pubspec for the package version identified by
   /// [id].
   ///
@@ -129,8 +187,8 @@ abstract class Source {
   /// Sources should not override this. Instead, they implement [doDescribe].
   Future<Pubspec> describe(PackageId id) async {
     if (id.isRoot) throw new ArgumentError("Cannot describe the root package.");
-    if (id.source != name) {
-      throw new ArgumentError("Package $id does not use source $name.");
+    if (id.source != source.name) {
+      throw new ArgumentError("Package $id does not use source ${source.name}.");
     }
 
     var pubspec = _pubspecs[id];
@@ -165,69 +223,10 @@ abstract class Source {
   /// If the source is cached, this will be a path in the system cache.
   String getDirectory(PackageId id);
 
-  /// Parses a [PackageRef] from a name and a user-provided [description].
-  ///
-  /// When a [Pubspec] is parsed, it reads in the description for each
-  /// dependency. It is up to the dependency's [Source] to determine how that
-  /// should be interpreted. This will be called during parsing to validate that
-  /// the given [description] is well-formed according to this source, and to
-  /// give the source a chance to canonicalize the description.
-  ///
-  /// [containingPath] is the path to the local file (pubspec or lockfile)
-  /// where this description appears. It may be `null` if the description is
-  /// coming from some in-memory source (such as pulling down a pubspec from
-  /// pub.dartlang.org).
-  ///
-  /// The description in the returned [PackageRef] need bear no resemblance to
-  /// the original user-provided description.
-  ///
-  /// Throws a [FormatException] if the description is not valid.
-  PackageRef parseRef(String name, description, {String containingPath});
-
-  /// Parses a [PackageId] from a name and a serialized description.
-  ///
-  /// This only accepts descriptions serialized using [serializeDescription]. It
-  /// should not be used with user-authored descriptions.
-  ///
-  /// Throws a [FormatException] if the description is not valid.
-  PackageId parseId(String name, Version version, description);
-
-  /// When a [LockFile] is serialized, it uses this method to get the
-  /// [description] in the right format.
-  ///
-  /// [containingPath] is the containing directory of the root package.
-  dynamic serializeDescription(String containingPath, description) {
-    return description;
-  }
-
-  /// When a package [description] is shown to the user, this is called to
-  /// convert it into a human-friendly form.
-  ///
-  /// By default, it just converts the description to a string, but sources
-  /// may customize this. [containingPath] is the containing directory of the
-  /// root package.
-  String formatDescription(String containingPath, description) {
-    return description.toString();
-  }
-
-  /// Returns whether or not [description1] describes the same package as
-  /// [description2] for this source.
-  ///
-  /// This method should be light-weight. It doesn't need to validate that
-  /// either package exists.
-  ///
-  /// Note that either description may be a reference description or an ID
-  /// description; they need not be the same type. ID descriptions should be
-  /// considered equal to the reference descriptions that produced them.
-  bool descriptionsEqual(description1, description2);
-
   /// Stores [pubspec] so it's returned when [describe] is called with [id].
   ///
   /// This is notionally protected; it should only be called by subclasses.
   void memoizePubspec(PackageId id, Pubspec pubspec) {
     _pubspecs[id] = pubspec;
   }
-
-  /// Returns the source's name.
-  String toString() => name;
 }