Add more metadata to non-programmatic barback exceptions.

Add more metadata to non-programmatic barback exceptions.

This also adds a BarbackException superclass for errors that come from the
package graph or from transformers.

R=rnystrom@google.com

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

[Bob Nystrom, 2013-08-13 00:04:29]

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/build_res...
File pkg/barback/lib/src/build_result.dart (right):

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/build_res...
pkg/barback/lib/src/build_result.dart:20: final Set<BarbackException> errors;
Is there a reason to not make it a list and preserve the order that the errors
occurred?

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/errors.dart
File pkg/barback/lib/src/errors.dart (right):

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/errors.da...
pkg/barback/lib/src/errors.dart:54: /// name.
"with the wrong..." -> "to a different package than the primary input's."

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/errors.da...
pkg/barback/lib/src/errors.dart:63: "${transform.primaryId.package}.";
Can you explain in the error message *why* it must be this and mention the
primary input ID?

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/errors.da...
pkg/barback/lib/src/errors.dart:96: /// [primaryId].
This is worded a bit strangely. How about:

Identifies a single transformation in the barback graph.

A transformation is uniquely identified by the ID of its primary input, and the
transformer that is applied to it.

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/phase.dart
File pkg/barback/lib/src/phase.dart (right):

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:79: final _outputs = new Map<AssetId,
List<Pair<AssetNode, TransformNode>>>();
Using a Pair here makes the code pretty hairy. How about just giving AssetNode a
reference to the TransformNode that generated it (and leaving it null for
sources)?

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:341: if (_outputs[collision].length <= 1) return;
Shouldn't this still be continue?

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

https://codereview.chromium.org/22961002/diff/1/pkg/barback/test/utils.dart#n...
pkg/barback/test/utils.dart:301: /// output of [transformation] to its input.
Mention [matcher] is wrapped too.

[nweiz, 2013-08-13 19:15:11]

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/build_res...
File pkg/barback/lib/src/build_result.dart (right):

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/build_res...
pkg/barback/lib/src/build_result.dart:20: final Set<BarbackException> errors;
On 2013/08/13 00:04:29, Bob Nystrom wrote:
> Is there a reason to not make it a list and preserve the order that the errors
> occurred?

The errors from different cascades all get merged together, so the order ends up
ultimately being erased anyway.

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/errors.dart
File pkg/barback/lib/src/errors.dart (right):

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/errors.da...
pkg/barback/lib/src/errors.dart:54: /// name.
On 2013/08/13 00:04:29, Bob Nystrom wrote:
> "with the wrong..." -> "to a different package than the primary input's."

Done.

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/errors.da...
pkg/barback/lib/src/errors.dart:63: "${transform.primaryId.package}.";
On 2013/08/13 00:04:29, Bob Nystrom wrote:
> Can you explain in the error message *why* it must be this and mention the
> primary input ID?

I've added data about the transforms to all the error messages.

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/errors.da...
pkg/barback/lib/src/errors.dart:96: /// [primaryId].
On 2013/08/13 00:04:29, Bob Nystrom wrote:
> This is worded a bit strangely. How about:
> 
> Identifies a single transformation in the barback graph.
> 
> A transformation is uniquely identified by the ID of its primary input, and
the
> transformer that is applied to it.

Done.

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/phase.dart
File pkg/barback/lib/src/phase.dart (right):

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:79: final _outputs = new Map<AssetId,
List<Pair<AssetNode, TransformNode>>>();
On 2013/08/13 00:04:29, Bob Nystrom wrote:
> Using a Pair here makes the code pretty hairy. How about just giving AssetNode
a
> reference to the TransformNode that generated it (and leaving it null for
> sources)?

Good idea. This will make it easier to do analysis on the package graph in the
future, too.

https://codereview.chromium.org/22961002/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:341: if (_outputs[collision].length <= 1) return;
On 2013/08/13 00:04:29, Bob Nystrom wrote:
> Shouldn't this still be continue?

Yes, merge error.

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

https://codereview.chromium.org/22961002/diff/1/pkg/barback/test/utils.dart#n...
pkg/barback/test/utils.dart:301: /// output of [transformation] to its input.
On 2013/08/13 00:04:29, Bob Nystrom wrote:
> Mention [matcher] is wrapped too.

This isn't done in any of the unittest documentation. It's implicit in operator
matchers.

[Bob Nystrom, 2013-08-13 20:15:46]

LGTM!

[nweiz, 2013-08-14 18:12:22]

Committed patchset #4 manually as r26133 (presubmit successful).