Refactor Source.

Refactor Source.

R=nweiz@google.com

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

[Bob Nystrom, 2014-04-18 20:07:26]

This has two main changes:

1. Define an UnknownSource that gets returned by SourceRegistry. That way,
callers don't have to check for null.

2. Split out all of the behavior of cached sources into a separate CachedSource
base class.

[nweiz, 2014-04-21 21:18:19]

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/pubspec.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/pubspec.dart:383: var source =
_sources[sourceName];
Nit: it's a little weird that you're declaring [source] so far away from its
only use.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source.dart:17: /// source is responsible for
getting these packages into the package cache.
Mention cached sources in this doc comment.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source.dart:73: return onDescribe(id);
We should come up with a more consistent naming convention for the pattern of
"the version of a method you're supposed to actually override." We use "onX"
here, "doX" elsewhere, and other idiosyncratic formulations in other places.

What do you think about "X_"? The underscore mirrors "_X", which may help
communicate that it's not supposed to be called by external classes, and it
won't conflict with any other naming conventions.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source.dart:76: Future<Pubspec>
onDescribe(PackageId id);
This should have documentation at least mentioning that it's the version of
[describe] intended to be overridden.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/source/cached.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:20: /// packages. It doesn't
need to be cached if all packages are available
A source should also be cached if the version of the code should be frozen on
install, e.g. with local git repositories.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:43: /// [id]. This will only be
called for packages that have not yet been
Single-sentence first paragraph.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:45: Future<Pubspec>
onDescribeUncached(PackageId id);
The use of the "on" convention is confusing here, since there's not a
corresponding [describeUncached] method.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:48: return
systemCache.download(id).then(
It's weird that this calls [systemCache] which then calls right back to
[source]. I get that [SystemCache] has the logic for merging multiple downloads,
but is that logic really necessary? There's no logic there for ensuring that it
doesn't download on top of an existing package, and my local testing also
indicates that the logic is never used. I suggest we replace that logic with an
assertion that we aren't making parallel requests, and have this code just call
[downloadToSystemCache] directly.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:53: Future<String>
getDirectory(PackageId id) => systemCacheDirectory(id);
Why even have [systemCacheDirectory] if [getDirectory], which was already
abstract in the superclass, just forwards to it?

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:72: /// [id] should be
downloaded to. This should return a path to a subdirectory
Single-sentence first paragraph.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:95: ///   * It has an empty
"lib" directory.
Currently we don't validate that a package has a lib/ directory on publish,
because we considered a package valid if it had assets. We don't really support
that use-case anymore, but soon we'll support a package that just has Dart files
in bin/, so it seems like maybe we shouldn't look for lib/ here anymore.

If we keep looking for lib/, we should re-add the validation, since publishing a
package that will constantly look corrupted is no good.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/source/hosted.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/hosted.dart:92: }
Why isn't this logic in CachedSource? We want to redownload corrupted git repos,
don't we?

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/source/unknown.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/unknown.dart:39: "Cannot find a package
from an unknown source.");
It would be nice to include the source's name in these error messages.

[Bob Nystrom, 2014-04-22 21:35:08]

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/pubspec.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/pubspec.dart:383: var source =
_sources[sourceName];
On 2014/04/21 21:18:19, nweiz wrote:
> Nit: it's a little weird that you're declaring [source] so far away from its
> only use.

Done.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source.dart:17: /// source is responsible for
getting these packages into the package cache.
On 2014/04/21 21:18:19, nweiz wrote:
> Mention cached sources in this doc comment.

Done.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source.dart:73: return onDescribe(id);
On 2014/04/21 21:18:19, nweiz wrote:
> We should come up with a more consistent naming convention for the pattern of
> "the version of a method you're supposed to actually override." We use "onX"
> here, "doX" elsewhere, and other idiosyncratic formulations in other places.

Yeah, we should be more consistent.

> What do you think about "X_"? The underscore mirrors "_X", which may help
> communicate that it's not supposed to be called by external classes, and it
> won't conflict with any other naming conventions.

Hmm, I'm not a fan of using punctuation for this.  Way back in the day, I worked
on projects in C++ that used "on___" for virtual methods, which is why I'm doing
that here. I like this convention but I would be up for something else too.

We could do something very explicit: derivedDescribe() or overriddenDescribe()?
Or maybe just try to come up with a name that distinguishes it from the base
method. For example, here it could be something like "describeValidPackage"
since describe() has done the validation.

Here's a discussion about it:
http://stackoverflow.com/questions/2597556/naming-convention-for-non-virtual-...

Herb Sutter uses a "Do" prefix.

Thoughts?

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source.dart:76: Future<Pubspec>
onDescribe(PackageId id);
On 2014/04/21 21:18:19, nweiz wrote:
> This should have documentation at least mentioning that it's the version of
> [describe] intended to be overridden.

Done.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/source/cached.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:20: /// packages. It doesn't
need to be cached if all packages are available
On 2014/04/21 21:18:19, nweiz wrote:
> A source should also be cached if the version of the code should be frozen on
> install, e.g. with local git repositories.

Done.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:43: /// [id]. This will only be
called for packages that have not yet been
On 2014/04/21 21:18:19, nweiz wrote:
> Single-sentence first paragraph.

Done.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:45: Future<Pubspec>
onDescribeUncached(PackageId id);
On 2014/04/21 21:18:19, nweiz wrote:
> The use of the "on" convention is confusing here, since there's not a
> corresponding [describeUncached] method.

Done.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:48: return
systemCache.download(id).then(
On 2014/04/21 21:18:19, nweiz wrote:
> It's weird that this calls [systemCache] which then calls right back to
> [source]. I get that [SystemCache] has the logic for merging multiple
downloads,
> but is that logic really necessary?

It turns out no! That code has been in there since the very first commit to
SystemCache before we even had a version solver, I think. Tore it out.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:53: Future<String>
getDirectory(PackageId id) => systemCacheDirectory(id);
On 2014/04/21 21:18:19, nweiz wrote:
> Why even have [systemCacheDirectory] if [getDirectory], which was already
> abstract in the superclass, just forwards to it?

Good point. Deleted.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:72: /// [id] should be
downloaded to. This should return a path to a subdirectory
On 2014/04/21 21:18:19, nweiz wrote:
> Single-sentence first paragraph.

Done.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:95: ///   * It has an empty
"lib" directory.
On 2014/04/21 21:18:19, nweiz wrote:
> Currently we don't validate that a package has a lib/ directory on publish,
> because we considered a package valid if it had assets. We don't really
support
> that use-case anymore, but soon we'll support a package that just has Dart
files
> in bin/, so it seems like maybe we shouldn't look for lib/ here anymore.
> 
> If we keep looking for lib/, we should re-add the validation, since publishing
a
> package that will constantly look corrupted is no good.

Yeah, I noticed that but didn't want to touch it in this patch. Now that we have
"pub cache repair" how do you feel about tearing the auto-repair out completely?
I don't know how many correct cases it actually catches.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/source/hosted.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/hosted.dart:92: }
On 2014/04/21 21:18:19, nweiz wrote:
> Why isn't this logic in CachedSource? We want to redownload corrupted git
repos,
> don't we?

This preserves the previous behavior. The code path was convoluted but as far as
I can tell, we didn't previously support auto-repairing git packages.

See my previous comment about removing this functionality entirely.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/source/unknown.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/unknown.dart:39: "Cannot find a package
from an unknown source.");
On 2014/04/21 21:18:19, nweiz wrote:
> It would be nice to include the source's name in these error messages.

Done.

[nweiz, 2014-04-22 23:50:03]

lgtm

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source.dart:73: return onDescribe(id);
On 2014/04/22 21:35:08, Bob Nystrom wrote:
> On 2014/04/21 21:18:19, nweiz wrote:
> > We should come up with a more consistent naming convention for the pattern
of
> > "the version of a method you're supposed to actually override." We use "onX"
> > here, "doX" elsewhere, and other idiosyncratic formulations in other places.
> 
> Yeah, we should be more consistent.
> 
> > What do you think about "X_"? The underscore mirrors "_X", which may help
> > communicate that it's not supposed to be called by external classes, and it
> > won't conflict with any other naming conventions.
> 
> Hmm, I'm not a fan of using punctuation for this.  Way back in the day, I
worked
> on projects in C++ that used "on___" for virtual methods, which is why I'm
doing
> that here. I like this convention but I would be up for something else too.
> 
> We could do something very explicit: derivedDescribe() or
overriddenDescribe()?
> Or maybe just try to come up with a name that distinguishes it from the base
> method. For example, here it could be something like "describeValidPackage"
> since describe() has done the validation.
> 
> Here's a discussion about it:
>
http://stackoverflow.com/questions/2597556/naming-convention-for-non-virtual-...
> 
> Herb Sutter uses a "Do" prefix.
> 
> Thoughts?
> 

Of these alternatives, I think I like "DoX" or choosing a new name best.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/source/cached.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:95: ///   * It has an empty
"lib" directory.
On 2014/04/22 21:35:08, Bob Nystrom wrote:
> On 2014/04/21 21:18:19, nweiz wrote:
> > Currently we don't validate that a package has a lib/ directory on publish,
> > because we considered a package valid if it had assets. We don't really
> support
> > that use-case anymore, but soon we'll support a package that just has Dart
> files
> > in bin/, so it seems like maybe we shouldn't look for lib/ here anymore.
> > 
> > If we keep looking for lib/, we should re-add the validation, since
publishing
> a
> > package that will constantly look corrupted is no good.
> 
> Yeah, I noticed that but didn't want to touch it in this patch. Now that we
have
> "pub cache repair" how do you feel about tearing the auto-repair out
completely?
> I don't know how many correct cases it actually catches.

I don't mind tearing out the auto-repair, but if we catch something really
egregious (e.g. a missing pubspec), we should at least print something to the
effect of "you should try running pub cache repair".

[Bob Nystrom, 2014-04-23 23:48:01]

Committed patchset #2 manually as r35344 (presubmit successful).

[Bob Nystrom, 2014-04-24 00:27:14]

Thanks!

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source.dart:73: return onDescribe(id);
On 2014/04/22 23:50:03, nweiz wrote:
> On 2014/04/22 21:35:08, Bob Nystrom wrote:
> > On 2014/04/21 21:18:19, nweiz wrote:
> > > We should come up with a more consistent naming convention for the pattern
> of
> > > "the version of a method you're supposed to actually override." We use
"onX"
> > > here, "doX" elsewhere, and other idiosyncratic formulations in other
places.
> > 
> > Yeah, we should be more consistent.
> > 
> > > What do you think about "X_"? The underscore mirrors "_X", which may help
> > > communicate that it's not supposed to be called by external classes, and
it
> > > won't conflict with any other naming conventions.
> > 
> > Hmm, I'm not a fan of using punctuation for this.  Way back in the day, I
> worked
> > on projects in C++ that used "on___" for virtual methods, which is why I'm
> doing
> > that here. I like this convention but I would be up for something else too.
> > 
> > We could do something very explicit: derivedDescribe() or
> overriddenDescribe()?
> > Or maybe just try to come up with a name that distinguishes it from the base
> > method. For example, here it could be something like "describeValidPackage"
> > since describe() has done the validation.
> > 
> > Here's a discussion about it:
> >
>
http://stackoverflow.com/questions/2597556/naming-convention-for-non-virtual-...
> > 
> > Herb Sutter uses a "Do" prefix.
> > 
> > Thoughts?
> > 
> 
> Of these alternatives, I think I like "DoX" or choosing a new name best.

Done. DoX it is.

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/source/cached.dart (right):

https://codereview.chromium.org/243683002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/cached.dart:95: ///   * It has an empty
"lib" directory.
On 2014/04/22 23:50:03, nweiz wrote:
> On 2014/04/22 21:35:08, Bob Nystrom wrote:
> > On 2014/04/21 21:18:19, nweiz wrote:
> > > Currently we don't validate that a package has a lib/ directory on
publish,
> > > because we considered a package valid if it had assets. We don't really
> > support
> > > that use-case anymore, but soon we'll support a package that just has Dart
> > files
> > > in bin/, so it seems like maybe we shouldn't look for lib/ here anymore.
> > > 
> > > If we keep looking for lib/, we should re-add the validation, since
> publishing
> > a
> > > package that will constantly look corrupted is no good.
> > 
> > Yeah, I noticed that but didn't want to touch it in this patch. Now that we
> have
> > "pub cache repair" how do you feel about tearing the auto-repair out
> completely?
> > I don't know how many correct cases it actually catches.
> 
> I don't mind tearing out the auto-repair, but if we catch something really
> egregious (e.g. a missing pubspec), we should at least print something to the
> effect of "you should try running pub cache repair".

I don't know if the missing pubspec case even happens in practice now. I think
the only time things get broken is when users walk into their package symlink,
which will skip past the pubspect and go into lib.

I'll just delete this completely for now and if we hear complaints about missing
pubspecs, I'm totally up for adding checking back in. Until then, I like the
idea of simplifying the codebase.