Start sketching out a buildAll() method.

Start sketching out a buildAll() method.

BUG=
R=nweiz@google.com

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

[Bob Nystrom, 2013-08-17 00:11:00]

This still needs tests and better docs, but what do you think about this
approach?

[Bob Nystrom, 2013-08-19 21:58:54]

This has tests now and actually works. It also patches in your in-flight change,
so you can ignore stuff that looks suspiciously familiar. PTAL!

[nweiz, 2013-08-20 00:32:24]

On 2013/08/19 21:58:54, Bob Nystrom wrote:
> This has tests now and actually works. It also patches in your in-flight
change,
> so you can ignore stuff that looks suspiciously familiar. PTAL!

Now that I've got my change in, can you merge and re-upload this so it's easier
to tell what's new?

[Bob Nystrom, 2013-08-20 16:23:40]

Rebased and fixed a bit. PTAL!

[nweiz, 2013-08-20 19:59:26]

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/barba...
File pkg/barback/lib/src/barback.dart (right):

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/barba...
pkg/barback/lib/src/barback.dart:96: /// returned future will complete with an
error if the build is not
"with an error" -> "with a [BarbackException]"

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/error...
File pkg/barback/lib/src/errors.dart (right):

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:23: /// Recursively removes any occurrences of
[AggregateException] in [errors]
"removes" -> "replaces"

Doing this recursively seems unnecessary, since AggregateException is guaranteed
not to contain any other AggregateExceptions.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:40: /// It implicitly flattens any
[AggregateException]s that occur in the list
"list exceptions" -> "list of exceptions"

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:43: final List<BarbackException> errors;
It seems like this should be a set, since the order exceptions come in is going
to be nondeterministic most of the time.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:47: }
It would be nice to have a static method here that takes an
Iterable<BarbackException> and does:

    if (errors.length == 0) return null;
    if (errors.length == 1) return errors.single;
    return new AggregateException(errors);

Also, this should have a useful toString.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/packa...
File pkg/barback/lib/src/package_graph.dart (right):

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/packa...
pkg/barback/lib/src/package_graph.dart:58: BarbackException _lastError;
I don't understand this. Why does the user only care about the errors from the
last cascade to complete? If they're getting all the assets, wouldn't they care
about all the errors from all the cascades? If so, why not just collect the
errors from _cascadeResults?

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/packa...
pkg/barback/lib/src/package_graph.dart:94: _lastError = error;
This is definitely not right. The error that's caught here almost certainly
won't be a BarbackException.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/packa...
pkg/barback/lib/src/package_graph.dart:121: if
(_cascadeResults.values.any((result) => result == null)) {
_cascadeResults.values.contains(null)

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/test/utils.dart
File pkg/barback/test/utils.dart (right):

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/test/utils.da...
pkg/barback/test/utils.dart:235: var id = new AssetId.parse(expected);
Parse these outside of [schedule] so that parse errors will be reported
synchronously and you can include them in the schedule description.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/test/utils.da...
pkg/barback/test/utils.dart:250: /// [match] should be a single matcher that the
error should match.
It seems weird to have a one-off special case for AggregateException. Eventually
we're going to want to start matching AggregateExceptions anywhere a
BarbackException might be thrown. It seems cleaner to create an
isAggregateException matcher.

That also makes this implementation a lot simpler:

    expect(_barback.getAllAssets(), throwsA(matcher))

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/test/utils.da...
pkg/barback/test/utils.dart:253: return
expect(_barback.getAllAssets().catchError((error) {
Returning expect doesn't do anything.

[Bob Nystrom, 2013-08-20 21:29:20]

Thanks!

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/barba...
File pkg/barback/lib/src/barback.dart (right):

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/barba...
pkg/barback/lib/src/barback.dart:96: /// returned future will complete with an
error if the build is not
On 2013/08/20 19:59:26, nweiz wrote:
> "with an error" -> "with a [BarbackException]"

Done.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/error...
File pkg/barback/lib/src/errors.dart (right):

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:23: /// Recursively removes any occurrences of
[AggregateException] in [errors]
On 2013/08/20 19:59:26, nweiz wrote:
> "removes" -> "replaces"
> 
> Doing this recursively seems unnecessary, since AggregateException is
guaranteed
> not to contain any other AggregateExceptions.

Done.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:40: /// It implicitly flattens any
[AggregateException]s that occur in the list
On 2013/08/20 19:59:26, nweiz wrote:
> "list exceptions" -> "list of exceptions"

Done.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:43: final List<BarbackException> errors;
On 2013/08/20 19:59:26, nweiz wrote:
> It seems like this should be a set, since the order exceptions come in is
going
> to be nondeterministic most of the time.

Done.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:47: }
On 2013/08/20 19:59:26, nweiz wrote:
> It would be nice to have a static method here that takes an
> Iterable<BarbackException> and does:
> 
>     if (errors.length == 0) return null;
>     if (errors.length == 1) return errors.single;
>     return new AggregateException(errors);
> 
> Also, this should have a useful toString.

Done.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/packa...
File pkg/barback/lib/src/package_graph.dart (right):

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/packa...
pkg/barback/lib/src/package_graph.dart:58: BarbackException _lastError;
On 2013/08/20 19:59:26, nweiz wrote:
> I don't understand this. Why does the user only care about the errors from the
> last cascade to complete?

They don't. This was aggregating all cascades.

> If they're getting all the assets, wouldn't they care
> about all the errors from all the cascades? If so, why not just collect the
> errors from _cascadeResults?

You're exactly right. Done.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/packa...
pkg/barback/lib/src/package_graph.dart:94: _lastError = error;
On 2013/08/20 19:59:26, nweiz wrote:
> This is definitely not right. The error that's caught here almost certainly
> won't be a BarbackException.

Removed.

I'm not sure what the best thing to do is here. In the case where you're calling
buildAllAssets(), you probably *aren't* listening to the streams. That means a
programmatic error will get dropped on the floor.

I think I'd prefer to pipe that through the resulting future if that occurs
after you've called buildAllAssets(), even though it isn't a BarbackException.
What do you think?

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/packa...
pkg/barback/lib/src/package_graph.dart:121: if
(_cascadeResults.values.any((result) => result == null)) {
On 2013/08/20 19:59:26, nweiz wrote:
> _cascadeResults.values.contains(null)

Done.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/test/utils.dart
File pkg/barback/test/utils.dart (right):

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/test/utils.da...
pkg/barback/test/utils.dart:235: var id = new AssetId.parse(expected);
On 2013/08/20 19:59:26, nweiz wrote:
> Parse these outside of [schedule] so that parse errors will be reported
> synchronously and you can include them in the schedule description.

Done.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/test/utils.da...
pkg/barback/test/utils.dart:250: /// [match] should be a single matcher that the
error should match.
On 2013/08/20 19:59:26, nweiz wrote:
> It seems weird to have a one-off special case for AggregateException.
Eventually
> we're going to want to start matching AggregateExceptions anywhere a
> BarbackException might be thrown. It seems cleaner to create an
> isAggregateException matcher.
> 
> That also makes this implementation a lot simpler:
> 
>     expect(_barback.getAllAssets(), throwsA(matcher))

Done. The implementation of isAggregateError() feels a bit wonky, but I don't
have a good feel for when I should compose matchers versus actually just writing
a matcher class from scratch.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/test/utils.da...
pkg/barback/test/utils.dart:253: return
expect(_barback.getAllAssets().catchError((error) {
On 2013/08/20 19:59:26, nweiz wrote:
> Returning expect doesn't do anything.

Done.

[nweiz, 2013-08-20 22:29:03]

A couple more comments, otherwise LGTM.

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/packa...
File pkg/barback/lib/src/package_graph.dart (right):

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/packa...
pkg/barback/lib/src/package_graph.dart:94: _lastError = error;
On 2013/08/20 21:29:20, Bob Nystrom wrote:
> On 2013/08/20 19:59:26, nweiz wrote:
> > This is definitely not right. The error that's caught here almost certainly
> > won't be a BarbackException.
> 
> Removed.
> 
> I'm not sure what the best thing to do is here. In the case where you're
calling
> buildAllAssets(), you probably *aren't* listening to the streams. That means a
> programmatic error will get dropped on the floor.
> 
> I think I'd prefer to pipe that through the resulting future if that occurs
> after you've called buildAllAssets(), even though it isn't a BarbackException.
> What do you think?

I think it's reasonable for [getAllAssets] to throw a non-BarbackException; the
documentation can explain the semantic difference between the two.

I think the best thing to do is to resurrect _lastError, but use it only for
programmatic errors. If there's been a programmatic error at any point,
[getAllAssets] should throw it, because it means that the package graph is in an
inconsistent state.

https://codereview.chromium.org/22824023/diff/18001/pkg/barback/lib/src/error...
File pkg/barback/lib/src/errors.dart (right):

https://codereview.chromium.org/22824023/diff/18001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:23: /// Recursively replaces any occurrences of
[AggregateException] in [errors]
Remove "Recursively"

https://codereview.chromium.org/22824023/diff/18001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:59: String toString() => "Multiple errors
occurred:\n\n- ${errors.join('\n- ')}";
We should be prepared for an individual error to be multiple lines long. Barback
already has [prefixLines] in utils, so you can use that.

https://codereview.chromium.org/22824023/diff/18001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:104: abstract class WrappedException implements
BarbackException {
This class should be private.

https://codereview.chromium.org/22824023/diff/18001/pkg/barback/test/utils.dart
File pkg/barback/test/utils.dart (right):

https://codereview.chromium.org/22824023/diff/18001/pkg/barback/test/utils.da...
pkg/barback/test/utils.dart:236: for (var expected in expected) {
I'm not a fan of shadowing this variable name.

[Bob Nystrom, 2013-08-21 18:09:49]

Committed patchset #6 manually as r26452 (presubmit successful).

[Bob Nystrom, 2013-08-21 18:10:23]

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/packa...
File pkg/barback/lib/src/package_graph.dart (right):

https://codereview.chromium.org/22824023/diff/12001/pkg/barback/lib/src/packa...
pkg/barback/lib/src/package_graph.dart:94: _lastError = error;
On 2013/08/20 22:29:03, nweiz wrote:
> On 2013/08/20 21:29:20, Bob Nystrom wrote:
> > On 2013/08/20 19:59:26, nweiz wrote:
> > > This is definitely not right. The error that's caught here almost
certainly
> > > won't be a BarbackException.
> > 
> > Removed.
> > 
> > I'm not sure what the best thing to do is here. In the case where you're
> calling
> > buildAllAssets(), you probably *aren't* listening to the streams. That means
a
> > programmatic error will get dropped on the floor.
> > 
> > I think I'd prefer to pipe that through the resulting future if that occurs
> > after you've called buildAllAssets(), even though it isn't a
BarbackException.
> > What do you think?
> 
> I think it's reasonable for [getAllAssets] to throw a non-BarbackException;
the
> documentation can explain the semantic difference between the two.
> 
> I think the best thing to do is to resurrect _lastError, but use it only for
> programmatic errors. If there's been a programmatic error at any point,
> [getAllAssets] should throw it, because it means that the package graph is in
an
> inconsistent state.

Done.

https://codereview.chromium.org/22824023/diff/18001/pkg/barback/lib/src/error...
File pkg/barback/lib/src/errors.dart (right):

https://codereview.chromium.org/22824023/diff/18001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:23: /// Recursively replaces any occurrences of
[AggregateException] in [errors]
On 2013/08/20 22:29:03, nweiz wrote:
> Remove "Recursively"

Done.

https://codereview.chromium.org/22824023/diff/18001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:59: String toString() => "Multiple errors
occurred:\n\n- ${errors.join('\n- ')}";
On 2013/08/20 22:29:03, nweiz wrote:
> We should be prepared for an individual error to be multiple lines long.
Barback
> already has [prefixLines] in utils, so you can use that.

Done.

https://codereview.chromium.org/22824023/diff/18001/pkg/barback/lib/src/error...
pkg/barback/lib/src/errors.dart:104: abstract class WrappedException implements
BarbackException {
On 2013/08/20 22:29:03, nweiz wrote:
> This class should be private.

Done.

https://codereview.chromium.org/22824023/diff/18001/pkg/barback/test/utils.dart
File pkg/barback/test/utils.dart (right):

https://codereview.chromium.org/22824023/diff/18001/pkg/barback/test/utils.da...
pkg/barback/test/utils.dart:236: for (var expected in expected) {
On 2013/08/20 22:29:03, nweiz wrote:
> I'm not a fan of shadowing this variable name.

Done. Mistake.