Add aggregate transformer types to barback.

Add aggregate transformer types to barback.

This doesn't make the types public yet. However, it does run non-aggregate
transformers using AggregateTransform.

R=rnystrom@google.com
BUG=18567
Committed: https://code.google.com/p/dart/source/detail?r=35864

[Bob Nystrom, 2014-05-05 23:41:56]

Thank you for breaking this stuff into more manageable patches!

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/aggregate_transform.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:26: /// The aggregate
key for this transform.
"Aggregate key" already means something to a lot of programmers (A key in SQL
made of more than one column), so this is confusing to me. What do you think of
just calling it a "transform key"? The Transform class already represents a
single application of a transformer to a set of inputs, so I think it's the
right idea.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:29: /// The stream of
primary inputs that have been aggregated for this transform.
"have been aggregated for" -> "will be processed by"?

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:34: /// transform
without external modification.
This is a bit hard to read in part because of the future tense. How about:

"The stream is closed when all inputs have been provided."

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:39: /// only those
inputs.
I think this is a bit misleading. If the transformer knows it will only have two
inputs, it should really do transform.primaryInputs.toList() and assert the
length is two. Doing take(2) communicates "there may be more but I only want the
first two", which isn't safe in barback since you have no idea what order your
primaries will appear.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:50: final
_allPrimaryInputs = new AssetSet();
"all" isn't very helpful here. How about "_emittedPrimaryInputs"?

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:116: if
(!_allPrimaryInputs.containsId(id)) {
What if you know what the primary input *will* be and want to consume it even
though it hasn't come through the stream yet?

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/aggregate_transformer.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transformer.dart:32: /// Any value
that can be passed across isolate boundaries can be returned. If
Really? I thought we were just going to use an AssetId for this.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transformer.dart:38: /// call.
It might help clarify to say this is equivalent to isPrimary returning false in
a non-aggregate transformer.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart:16: class
DeclaringAggregateTransform extends BaseTransform {
A bunch of my comments on AggregateTransform apply here too.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart:34: ///
access only those inputs' ids.
This seems like it would be more helpful as a doc comment for
DeclaringTransformer.declareOutputs() instead of here.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart:50:
_idController.stream.listen(_allPrimaryId.add);
_allPrimaryIds

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart:59: ///
[DeclaringTransformer.declareOutputs] declareds a given asset id for a
"declareds" -> "declares".

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart:64: // has
already been declared by this transformer.
Is there a reason we want to throw for this instead of just ignoring it?

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/lazy_aggregate_transformer.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/lazy_aggregate_transformer.dart:16: abstract
class LazyAggregateTransformer extends DeclaringAggregateTransformer {}
It seems like the Transformer class hierarchy is starting to get combinatorial.
It's six classes now, right?

If we find ourselves needing another flavor of Transformer class, we may want to
reconsider splitting Declaring out to a separate interface and Lazy to a tag
interface or annotation.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/transform.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/transform.dart:16: /// Creates a new [Transform]
wrapping an [AggregateTransform].
It would be more helpful to document this in terms of its intent. Something
like: "provides an implementation of the more user-friendly Transformer
interface in terms of the fundamental AggregateTransform interface that barback
maps everything to."

Also, can you explain for our future selves why it doesn't have to worry about
.primaryInputs producing more than one value?

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/transform.dart:30: final AggregateTransform
_aggregate;
Doc comment.

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

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/utils.da...
pkg/barback/lib/src/utils.dart:319: // TODO(nweiz): Implement this using a
transformer when issues 18588 and 18586
"transformer" -> "stream transformer".

[nweiz, 2014-05-06 22:46:40]

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/aggregate_transform.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:26: /// The aggregate
key for this transform.
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> "Aggregate key" already means something to a lot of programmers (A key in SQL
> made of more than one column), so this is confusing to me. What do you think
of
> just calling it a "transform key"? The Transform class already represents a
> single application of a transformer to a set of inputs, so I think it's the
> right idea.

Done.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:29: /// The stream of
primary inputs that have been aggregated for this transform.
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> "have been aggregated for" -> "will be processed by"?

Done.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:34: /// transform
without external modification.
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> This is a bit hard to read in part because of the future tense. How about:
> 
> "The stream is closed when all inputs have been provided."

I want to be explicit that the stream will stay open even after all assets have
been provided since it might still take some time to prove that there are no
more assets.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:39: /// only those
inputs.
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> I think this is a bit misleading. If the transformer knows it will only have
two
> inputs, it should really do transform.primaryInputs.toList() and assert the
> length is two. Doing take(2) communicates "there may be more but I only want
the
> first two", which isn't safe in barback since you have no idea what order your
> primaries will appear.

`toList()` is definitely not what you want to do in this situation, since it
will block unnecessarily until barback can prove that there are no additional
assets to be processed. Unfortunately, there's no method on Stream that says
"take N and assert that nothing else is emitted", but I think `take(2)` is a
fine way to write this.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:50: final
_allPrimaryInputs = new AssetSet();
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> "all" isn't very helpful here. How about "_emittedPrimaryInputs"?

Done.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:116: if
(!_allPrimaryInputs.containsId(id)) {
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> What if you know what the primary input *will* be and want to consume it even
> though it hasn't come through the stream yet?

You really shouldn't be assuming that an asset exists without having been passed
it explicitly; that's a recipe for failure.

Even if there are some narrow cases where this is possible to do safely, I think
providing useful and immediate feedback when someone's doing something wrong is
more important than supporting them.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/aggregate_transformer.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transformer.dart:32: /// Any value
that can be passed across isolate boundaries can be returned. If
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> Really? I thought we were just going to use an AssetId for this.

That wasn't my plan. It also seems like a lot of users will be writing something
like "new AssetId(id.package, theThingIActuallyWantToReturn)".

There's no utility in allowing users to specify a different package in the ID,
since all assets will be coming from the same package, so it just seems like it
would make more work for them and encode a bunch of our assumptions about what
types of classification are likely to exist.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transformer.dart:38: /// call.
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> It might help clarify to say this is equivalent to isPrimary returning false
in
> a non-aggregate transformer.

Done.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart:16: class
DeclaringAggregateTransform extends BaseTransform {
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> A bunch of my comments on AggregateTransform apply here too.

Done.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart:34: ///
access only those inputs' ids.
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> This seems like it would be more helpful as a doc comment for
> DeclaringTransformer.declareOutputs() instead of here.

I'll put it both places.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart:50:
_idController.stream.listen(_allPrimaryId.add);
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> _allPrimaryIds

Done.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart:59: ///
[DeclaringTransformer.declareOutputs] declareds a given asset id for a
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> "declareds" -> "declares".

Done.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart:64: // has
already been declared by this transformer.
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> Is there a reason we want to throw for this instead of just ignoring it?

It's effectively a collision—the user has provided two separate outputs with the
same id, which is an error anywhere else. The benefit is that here we can
complain about it immediately when it happens.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/lazy_aggregate_transformer.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/lazy_aggregate_transformer.dart:16: abstract
class LazyAggregateTransformer extends DeclaringAggregateTransformer {}
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> It seems like the Transformer class hierarchy is starting to get
combinatorial.
> It's six classes now, right?
> 
> If we find ourselves needing another flavor of Transformer class, we may want
to
> reconsider splitting Declaring out to a separate interface and Lazy to a tag
> interface or annotation.

That doesn't sound like an important enough breaking change to warrant
supporting an older version of barback in pub in perpetuity.

I don't think it would even provide much benefit. The class hierarchy encodes
things we want: DeclaringTransformer and DeclaringAggregateTransformer add
different methods than one another, and Lazy* interfaces require that their
respective declare* methods are implemented.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/transform.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/transform.dart:16: /// Creates a new [Transform]
wrapping an [AggregateTransform].
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> It would be more helpful to document this in terms of its intent. Something
> like: "provides an implementation of the more user-friendly Transformer
> interface in terms of the fundamental AggregateTransform interface that
barback
> maps everything to."
> 
> Also, can you explain for our future selves why it doesn't have to worry about
> .primaryInputs producing more than one value?

Done.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/transform.dart:30: final AggregateTransform
_aggregate;
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> Doc comment.

Done.

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

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/utils.da...
pkg/barback/lib/src/utils.dart:319: // TODO(nweiz): Implement this using a
transformer when issues 18588 and 18586
On 2014/05/05 23:41:56, Bob Nystrom wrote:
> "transformer" -> "stream transformer".

Done.

[Bob Nystrom, 2014-05-06 23:55:08]

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/aggregate_transform.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:34: /// transform
without external modification.
On 2014/05/06 22:46:40, nweiz wrote:
> On 2014/05/05 23:41:56, Bob Nystrom wrote:
> > This is a bit hard to read in part because of the future tense. How about:
> > 
> > "The stream is closed when all inputs have been provided."
> 
> I want to be explicit that the stream will stay open even after all assets
have
> been provided since it might still take some time to prove that there are no
> more assets.

Maybe just say that then: "The stream is closed not just when all inputs are
provided, but when barback is sure no more inputs will are forthcoming."

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:39: /// only those
inputs.
On 2014/05/06 22:46:40, nweiz wrote:
> On 2014/05/05 23:41:56, Bob Nystrom wrote:
> > I think this is a bit misleading. If the transformer knows it will only have
> two
> > inputs, it should really do transform.primaryInputs.toList() and assert the
> > length is two. Doing take(2) communicates "there may be more but I only want
> the
> > first two", which isn't safe in barback since you have no idea what order
your
> > primaries will appear.
> 
> `toList()` is definitely not what you want to do in this situation, since it
> will block unnecessarily until barback can prove that there are no additional
> assets to be processed.

Oof, we should discuss this, because I think that definitely what users will
naturally think to do. If that's the wrong practice, we're setting them up for
failure.

I do understand what you're saying, I just wonder if there's some API tweaks we
can do to make this more intuitive.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:116: if
(!_allPrimaryInputs.containsId(id)) {
On 2014/05/06 22:46:40, nweiz wrote:
> On 2014/05/05 23:41:56, Bob Nystrom wrote:
> > What if you know what the primary input *will* be and want to consume it
even
> > though it hasn't come through the stream yet?
> 
> You really shouldn't be assuming that an asset exists without having been
passed
> it explicitly; that's a recipe for failure.
> 
> Even if there are some narrow cases where this is possible to do safely, I
think
> providing useful and immediate feedback when someone's doing something wrong
is
> more important than supporting them.

SGTM.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/aggregate_transformer.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transformer.dart:32: /// Any value
that can be passed across isolate boundaries can be returned. If
On 2014/05/06 22:46:40, nweiz wrote:
> On 2014/05/05 23:41:56, Bob Nystrom wrote:
> > Really? I thought we were just going to use an AssetId for this.
> 
> That wasn't my plan. It also seems like a lot of users will be writing
something
> like "new AssetId(id.package, theThingIActuallyWantToReturn)".
> 
> There's no utility in allowing users to specify a different package in the ID,
> since all assets will be coming from the same package, so it just seems like
it
> would make more work for them and encode a bunch of our assumptions about what
> types of classification are likely to exist.

Per our discussion, let's make this a string and document it should be
path-like.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart:64: // has
already been declared by this transformer.
On 2014/05/06 22:46:40, nweiz wrote:
> On 2014/05/05 23:41:56, Bob Nystrom wrote:
> > Is there a reason we want to throw for this instead of just ignoring it?
> 
> It's effectively a collision—the user has provided two separate outputs with
the
> same id, which is an error anywhere else. The benefit is that here we can
> complain about it immediately when it happens.

I suppose so, but part of the reason we care about asset collisions is because
you could output two *different* assets with the same ID. There's no right way
to handle that.

In this case, it's just a pure ID, so outputting it twice doesn't cause any loss
of information.

I don't have a strong preference here, though, so do it how you like.

[nweiz, 2014-05-07 01:28:50]

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/aggregate_transform.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:34: /// transform
without external modification.
On 2014/05/06 23:55:09, Bob Nystrom wrote:
> On 2014/05/06 22:46:40, nweiz wrote:
> > On 2014/05/05 23:41:56, Bob Nystrom wrote:
> > > This is a bit hard to read in part because of the future tense. How about:
> > > 
> > > "The stream is closed when all inputs have been provided."
> > 
> > I want to be explicit that the stream will stay open even after all assets
> have
> > been provided since it might still take some time to prove that there are no
> > more assets.
> 
> Maybe just say that then: "The stream is closed not just when all inputs are
> provided, but when barback is sure no more inputs will are forthcoming."
> 

Done.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transform.dart:39: /// only those
inputs.
On 2014/05/06 23:55:09, Bob Nystrom wrote:
> On 2014/05/06 22:46:40, nweiz wrote:
> > On 2014/05/05 23:41:56, Bob Nystrom wrote:
> > > I think this is a bit misleading. If the transformer knows it will only
have
> > two
> > > inputs, it should really do transform.primaryInputs.toList() and assert
the
> > > length is two. Doing take(2) communicates "there may be more but I only
want
> > the
> > > first two", which isn't safe in barback since you have no idea what order
> your
> > > primaries will appear.
> > 
> > `toList()` is definitely not what you want to do in this situation, since it
> > will block unnecessarily until barback can prove that there are no
additional
> > assets to be processed.
> 
> Oof, we should discuss this, because I think that definitely what users will
> naturally think to do. If that's the wrong practice, we're setting them up for
> failure.
> 
> I do understand what you're saying, I just wonder if there's some API tweaks
we
> can do to make this more intuitive.

toList() will work just fine for 99% of aggregate transformers. I doubt many
aggregate transformers (other than auto-wrapped Transformers) will actually want
to consume a finite, known number of assets. I don't think we need to go out of
our way to make sure that they'll always do so in the most optimal way.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/aggregate_transformer.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transformer.dart:32: /// Any value
that can be passed across isolate boundaries can be returned. If
On 2014/05/06 23:55:09, Bob Nystrom wrote:
> On 2014/05/06 22:46:40, nweiz wrote:
> > On 2014/05/05 23:41:56, Bob Nystrom wrote:
> > > Really? I thought we were just going to use an AssetId for this.
> > 
> > That wasn't my plan. It also seems like a lot of users will be writing
> something
> > like "new AssetId(id.package, theThingIActuallyWantToReturn)".
> > 
> > There's no utility in allowing users to specify a different package in the
ID,
> > since all assets will be coming from the same package, so it just seems like
> it
> > would make more work for them and encode a bunch of our assumptions about
what
> > types of classification are likely to exist.
> 
> Per our discussion, let's make this a string and document it should be
> path-like.

Done.

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart (right):

https://codereview.chromium.org/262173009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/declaring_aggregate_transform.dart:64: // has
already been declared by this transformer.
On 2014/05/06 23:55:09, Bob Nystrom wrote:
> On 2014/05/06 22:46:40, nweiz wrote:
> > On 2014/05/05 23:41:56, Bob Nystrom wrote:
> > > Is there a reason we want to throw for this instead of just ignoring it?
> > 
> > It's effectively a collision—the user has provided two separate outputs with
> the
> > same id, which is an error anywhere else. The benefit is that here we can
> > complain about it immediately when it happens.
> 
> I suppose so, but part of the reason we care about asset collisions is because
> you could output two *different* assets with the same ID. There's no right way
> to handle that.
> 
> In this case, it's just a pure ID, so outputting it twice doesn't cause any
loss
> of information.
> 
> I don't have a strong preference here, though, so do it how you like.

In the non-declaring transformer, though, it's not a pure ID, and I think it's
nice to have behavioral symmetry here.

[Bob Nystrom, 2014-05-07 15:45:01]

Comments SGTM. LGTM!

[nweiz, 2014-05-07 20:09:04]

Committed patchset #5 manually as r35864 (presubmit successful).