Refactor Source and SourceRegistry.

Refactor Source and SourceRegistry.

This splits Source into two classes, Source and LiveSource. The former
is completely independent of the system cache directory, and mostly
contains source metadata and logic about manipulating descriptions.

SourceRegistry now deals strictly in Sources, with LiveSources being
handled by SystemCache. This means that classes like Pubspec and
LockFile no longer have transitive references to the system cache
directory.

This will also make it possible to clean up PackageName equality logic
by making PackageNames track their concrete Sources rather than just
their source names.

See #912

R=rnystrom@google.com

Committed: https://github.com/dart-lang/pub/commit/3fb39388c4e1cd872a83ca971f458e0976c758e0

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

https://codereview.chromium.org/2044253003/diff/20001/lib/src/entrypoint.dart
File lib/src/entrypoint.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/entrypoint.dart...
lib/src/entrypoint.dart:93: value: (id) => cache.load(id));
value: cache.load

?

https://codereview.chromium.org/2044253003/diff/20001/lib/src/solver/backtrac...
File lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/solver/backtrac...
lib/src/solver/backtracking_solver.dart:131:
systemCache.sources.hosted.refFor(name).withConstraint(constraint));
Long line.

https://codereview.chromium.org/2044253003/diff/20001/lib/src/solver/backtrac...
lib/src/solver/backtracking_solver.dart:135: })() {
Stuffing a giant immediately invoked function in here is kind of gnarly. How
about making this a factory constructor instead?

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source.dart
File lib/src/source.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source.dart#new...
lib/src/source.dart:132: abstract class LiveSource {
"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.

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source/hosted.dart
File lib/src/source/hosted.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source/hosted.d...
lib/src/source/hosted.dart:115: /// The bound version of [HostedSource].
"Bound" isn't used elsewhere to describe live sources.

You can probably just delete the doc comment.

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source/hosted.d...
lib/src/source/hosted.dart:325: var scheme = match[1] == 'https://' ||
localhost.isNotEmpty ? '' : match[1];
Long line.

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source_registry...
File lib/src/source_registry.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source_registry...
lib/src/source_registry.dart:30: List<Source> get sources {
What do you think of calling this "all"?

Elsewhere, we tend to store a reference to the registry in variable named
"sources", and "sources.sources" looks weird.

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

https://codereview.chromium.org/2044253003/diff/20001/lib/src/entrypoint.dart
File lib/src/entrypoint.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/entrypoint.dart...
lib/src/entrypoint.dart:93: value: (id) => cache.load(id));
On 2016/06/14 23:21:55, Bob Nystrom wrote:
> value: cache.load
> 
> ?

I really don't like that pattern in general. It makes stack traces a lot more
confusing.

https://codereview.chromium.org/2044253003/diff/20001/lib/src/solver/backtrac...
File lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/solver/backtrac...
lib/src/solver/backtracking_solver.dart:131:
systemCache.sources.hosted.refFor(name).withConstraint(constraint));
On 2016/06/14 23:21:55, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/2044253003/diff/20001/lib/src/solver/backtrac...
lib/src/solver/backtracking_solver.dart:135: })() {
On 2016/06/14 23:21:55, Bob Nystrom wrote:
> Stuffing a giant immediately invoked function in here is kind of gnarly. How
> about making this a factory constructor instead?

Made it a static method instead.

I really hate using factory constructors for situations like this; it adds a
bunch of tight coupling where multiple methods need to be updated whenever a
constructor parameter changes, and it de-localizes the constructor logic.

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source.dart
File lib/src/source.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source.dart#new...
lib/src/source.dart:132: abstract class LiveSource {
On 2016/06/14 23:21:55, Bob Nystrom wrote:
> "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.

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

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source/hosted.dart
File lib/src/source/hosted.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source/hosted.d...
lib/src/source/hosted.dart:115: /// The bound version of [HostedSource].
On 2016/06/14 23:21:55, Bob Nystrom wrote:
> "Bound" isn't used elsewhere to describe live sources.
> 
> You can probably just delete the doc comment.

I don't like leaving classes undocumented :-/. What about "The [LiveSource] for
[HostedSource]?"

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source/hosted.d...
lib/src/source/hosted.dart:325: var scheme = match[1] == 'https://' ||
localhost.isNotEmpty ? '' : match[1];
On 2016/06/14 23:21:55, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source_registry...
File lib/src/source_registry.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source_registry...
lib/src/source_registry.dart:30: List<Source> get sources {
On 2016/06/14 23:21:55, Bob Nystrom wrote:
> What do you think of calling this "all"?
> 
> Elsewhere, we tend to store a reference to the registry in variable named
> "sources", and "sources.sources" looks weird.

Done.

[nweiz, 2016-06-20 20:47:02]

Code review changes

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

lgtm

https://codereview.chromium.org/2044253003/diff/20001/lib/src/entrypoint.dart
File lib/src/entrypoint.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/entrypoint.dart...
lib/src/entrypoint.dart:93: value: (id) => cache.load(id));
On 2016/06/20 20:46:08, nweiz wrote:
> On 2016/06/14 23:21:55, Bob Nystrom wrote:
> > value: cache.load
> > 
> > ?
> 
> I really don't like that pattern in general. It makes stack traces a lot more
> confusing.

Oh, really? The style guide actually encourages this. I didn't know anyone
didn't like it.

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source.dart
File lib/src/source.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source.dart#new...
lib/src/source.dart:132: abstract class LiveSource {
On 2016/06/20 20:46:08, nweiz wrote:
> On 2016/06/14 23:21:55, Bob Nystrom wrote:
> > "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.
> 
> "Provider" means more or less the same thing as "Source", which seems
confusing.

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.

> What about "BoundSource"?

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]

https://codereview.chromium.org/2044253003/diff/20001/lib/src/entrypoint.dart
File lib/src/entrypoint.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/entrypoint.dart...
lib/src/entrypoint.dart:93: value: (id) => cache.load(id));
On 2016/06/20 21:20:31, Bob Nystrom wrote:
> On 2016/06/20 20:46:08, nweiz wrote:
> > On 2016/06/14 23:21:55, Bob Nystrom wrote:
> > > value: cache.load
> > > 
> > > ?
> > 
> > I really don't like that pattern in general. It makes stack traces a lot
more
> > confusing.
> 
> Oh, really? The style guide actually encourages this. I didn't know anyone
> didn't like it.

https://github.com/dart-lang/www.dartlang.org/issues/1503

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source.dart
File lib/src/source.dart (right):

https://codereview.chromium.org/2044253003/diff/20001/lib/src/source.dart#new...
lib/src/source.dart:132: abstract class LiveSource {
On 2016/06/20 21:20:31, Bob Nystrom wrote:
> On 2016/06/20 20:46:08, nweiz wrote:
> > On 2016/06/14 23:21:55, Bob Nystrom wrote:
> > > "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.
> > 
> > "Provider" means more or less the same thing as "Source", which seems
> confusing.
> 
> 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.
> 
> > What about "BoundSource"?
> 
> 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.

Going with BoundSource as discussed offline.

[nweiz, 2016-06-20 22:17:58]

Description was changed from

==========
Refactor Source and SourceRegistry.

This splits Source into two classes, Source and LiveSource. The former
is completely independent of the system cache directory, and mostly
contains source metadata and logic about manipulating descriptions.

SourceRegistry now deals strictly in Sources, with LiveSources being
handled by SystemCache. This means that classes like Pubspec and
LockFile no longer have transitive references to the system cache
directory.

This will also make it possible to clean up PackageName equality logic
by making PackageNames track their concrete Sources rather than just
their source names.

See #912

R=rnystrom@google.com
==========

to

==========
Refactor Source and SourceRegistry.

This splits Source into two classes, Source and LiveSource. The former
is completely independent of the system cache directory, and mostly
contains source metadata and logic about manipulating descriptions.

SourceRegistry now deals strictly in Sources, with LiveSources being
handled by SystemCache. This means that classes like Pubspec and
LockFile no longer have transitive references to the system cache
directory.

This will also make it possible to clean up PackageName equality logic
by making PackageNames track their concrete Sources rather than just
their source names.

See #912

R=rnystrom@google.com

Committed:
https://github.com/dart-lang/pub/commit/3fb39388c4e1cd872a83ca971f458e0976c758e0
==========

[nweiz, 2016-06-20 22:17:59]

Committed patchset #4 (id:60001) manually as
3fb39388c4e1cd872a83ca971f458e0976c758e0 (presubmit successful).