Transition barback's infrastructure to an aggregate-based model.

Transition barback's infrastructure to an aggregate-based model.

Aggregate transformers still aren't exposed publicly, but the normal
transformers are now all wrapped in aggregate transformers.

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

[Bob Nystrom, 2014-05-08 20:30:47]

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

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/errors.d...
pkg/barback/lib/src/errors.dart:172: /// also return additional types in the
future.
I know we talked about this a bit before but I don't remember the details.

Is this class just used to associate logs with a specific transformer? If so, I
think it would be good to move to a dumber object here. I worry including the
actual object here is future-API-evolution hostile.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/errors.d...
pkg/barback/lib/src/errors.dart:176: final AssetId primaryId;
Mark this deprecated and add a String "key" getter?

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
File pkg/barback/lib/src/graph/transform_node.dart (right):

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:156: /// The controller for the
currently-running [AggregateTransformer.apply] call's
Long line.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:185: void addInput(AssetNode
input) {
addInput -> addPrimary?

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:387: _declareController =
controller;
No reason to make a local for this.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:397: controller.cancel();
Document that calling cancel here deliberately even if no failure/interruption
occurred because it's a superset of done().

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:406: if (controller.loggedError) {
Document something like: "If declaration fails, fall back to treating the
transformer like a non-declaring one."

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:485: 
Document this please. Something like: "If we got invalidated while applying,
restart unless the transformer is lazy."

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:505: for (var id in
_primaries.map((node) => node.id)) {
Nit: Using map() here seems a bit gratuitous. Why not just using ".id" in the
body, or make a little local variable?

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:542: /// Run [Transformer.apply]
as soon as [primary] is available.
Update doc comment.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:557: controller.cancel();
Document that doing this deliberately even on success.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:571: if (_state ==
_State.NEEDS_DECLARE) return false;
What if _state is DECLARING? Can that still happen?

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:618: _emitPassThrough(id);
What do you think of:

_emitPassThrough -> _passThrough
_dontEmitPassThrough -> _consumePrimary

Putting "emit" on there when "pass-through" is already a nouned verb seems
unnecessary. :)

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:690: /// transformer, mark
[_declareController] as done.
_applyController.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:715: /// If the set of primary
inputs changes while in this state, it will
Is this true of any change, or just modifications/removals of existing
primaries? Will a new primary appearing reset this?

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:746: /// [NEEDS_DECLARE]. If the
it's still in this state when `apply` finishes
"the it's" -> "it's"

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

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transformer.dart:39: /// [Future] that
completes to the key.
This last sentence seems redundant with the first. Maybe just reword the first
sentence to clarify the async case?

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

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/base_transform.dart:50: if (asset == null) asset
= _node.info.primaryId;
This is a "key" now right? How about renaming "asset" -> "key" here and
elsewhere?

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

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/wrapping_aggregate_transformer.dart:38:
transformer as DeclaringTransformer);
Are the "as" operators needed here? Seems like a bug.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/wrapping_aggregate_transformer.dart:68:
super._(transformer as Transformer);
Isn't this just an upcast? Why is it needed?

https://codereview.chromium.org/267393009/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/declaring_transformer_test.dart (right):

https://codereview.chromium.org/267393009/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/declaring_transformer_test.dart:202: // Make sure
we're blocking on [declaring.apply]
"."

[nweiz, 2014-05-08 21:12:36]

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

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/errors.d...
pkg/barback/lib/src/errors.dart:172: /// also return additional types in the
future.
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> I know we talked about this a bit before but I don't remember the details.
> 
> Is this class just used to associate logs with a specific transformer? If so,
I
> think it would be good to move to a dumber object here. I worry including the
> actual object here is future-API-evolution hostile.

I think it's useful for users to be able to reliably check whether a log entry
came from their transformer, and the best way to do that is to allow them to
write "myTransformer == logEntry.transform.transformer".

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/errors.d...
pkg/barback/lib/src/errors.dart:176: final AssetId primaryId;
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> Mark this deprecated and add a String "key" getter?

In the vast majority of cases this is going to be an actual primary id. I think
it would net add confusion to the API to have separate "package" and "key"
getters that are almost always going to just be the components of the primary id
anyway.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
File pkg/barback/lib/src/graph/transform_node.dart (right):

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:156: /// The controller for the
currently-running [AggregateTransformer.apply] call's
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:185: void addInput(AssetNode
input) {
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> addInput -> addPrimary?

Done.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:387: _declareController =
controller;
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> No reason to make a local for this.

The instance variable gets nulled out in the whenComplete block, but still needs
to be accessed after that. Between that and passing [callback] to a new
[_declareOutputs] call, it would be very hard to use the instance variable
everywhere in this method.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:397: controller.cancel();
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> Document that calling cancel here deliberately even if no failure/interruption
> occurred because it's a superset of done().

Done.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:406: if (controller.loggedError) {
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> Document something like: "If declaration fails, fall back to treating the
> transformer like a non-declaring one."

Done.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:485: 
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> Document this please. Something like: "If we got invalidated while applying,
> restart unless the transformer is lazy."

Done.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:505: for (var id in
_primaries.map((node) => node.id)) {
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> Nit: Using map() here seems a bit gratuitous. Why not just using ".id" in the
> body, or make a little local variable?

Done.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:542: /// Run [Transformer.apply]
as soon as [primary] is available.
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> Update doc comment.

Done.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:557: controller.cancel();
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> Document that doing this deliberately even on success.

Done.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:571: if (_state ==
_State.NEEDS_DECLARE) return false;
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> What if _state is DECLARING? Can that still happen?

There's no way to transition to DECLARING until `apply` has finished running.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:618: _emitPassThrough(id);
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> What do you think of:
> 
> _emitPassThrough -> _passThrough
> _dontEmitPassThrough -> _consumePrimary
> 
> Putting "emit" on there when "pass-through" is already a nouned verb seems
> unnecessary. :)

Done.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:690: /// transformer, mark
[_declareController] as done.
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> _applyController.

Done.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:715: /// If the set of primary
inputs changes while in this state, it will
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> Is this true of any change, or just modifications/removals of existing
> primaries? Will a new primary appearing reset this?

A new primary will only reset it if the id stream has been closed, but I don't
know if there's a terse way to say that here.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:746: /// [NEEDS_DECLARE]. If the
it's still in this state when `apply` finishes
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> "the it's" -> "it's"

Done.

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

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/aggregate_transformer.dart:39: /// [Future] that
completes to the key.
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> This last sentence seems redundant with the first. Maybe just reword the first
> sentence to clarify the async case?

Done.

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

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/base_transform.dart:50: if (asset == null) asset
= _node.info.primaryId;
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> This is a "key" now right? How about renaming "asset" -> "key" here and
> elsewhere?

See my comment on TransformInfo.primaryId.

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

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/wrapping_aggregate_transformer.dart:38:
transformer as DeclaringTransformer);
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> Are the "as" operators needed here? Seems like a bug.

The analyzer complained. I have no idea why, but filing bugs for this sort of
thing in the past has mostly led to heartache.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transformer/wrapping_aggregate_transformer.dart:68:
super._(transformer as Transformer);
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> Isn't this just an upcast? Why is it needed?

DeclaringTransformer doesn't actually implement Transformer.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/declaring_transformer_test.dart (right):

https://codereview.chromium.org/267393009/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/declaring_transformer_test.dart:202: // Make sure
we're blocking on [declaring.apply]
On 2014/05/08 20:30:48, Bob Nystrom wrote:
> "."

Done.

[Bob Nystrom, 2014-05-08 23:56:13]

LGTM!

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

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/errors.d...
pkg/barback/lib/src/errors.dart:172: /// also return additional types in the
future.
On 2014/05/08 21:12:36, nweiz wrote:
> On 2014/05/08 20:30:48, Bob Nystrom wrote:
> > I know we talked about this a bit before but I don't remember the details.
> > 
> > Is this class just used to associate logs with a specific transformer? If
so,
> I
> > think it would be good to move to a dumber object here. I worry including
the
> > actual object here is future-API-evolution hostile.
> 
> I think it's useful for users to be able to reliably check whether a log entry
> came from their transformer, and the best way to do that is to allow them to
> write "myTransformer == logEntry.transform.transformer".

Maybe, but I think we could support that with myTransformer.id ==
logEntry.transformer.id just as well.

But do what you like here.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
File pkg/barback/lib/src/graph/transform_node.dart (right):

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:387: _declareController =
controller;
On 2014/05/08 21:12:36, nweiz wrote:
> On 2014/05/08 20:30:48, Bob Nystrom wrote:
> > No reason to make a local for this.
> 
> The instance variable gets nulled out in the whenComplete block, but still
needs
> to be accessed after that. Between that and passing [callback] to a new
> [_declareOutputs] call, it would be very hard to use the instance variable
> everywhere in this method.

Ah, right. I didn't notice it was used down below.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:571: if (_state ==
_State.NEEDS_DECLARE) return false;
On 2014/05/08 21:12:36, nweiz wrote:
> On 2014/05/08 20:30:48, Bob Nystrom wrote:
> > What if _state is DECLARING? Can that still happen?
> 
> There's no way to transition to DECLARING until `apply` has finished running.

Add an assert above then?

[nweiz, 2014-05-19 21:11:08]

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

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/errors.d...
pkg/barback/lib/src/errors.dart:172: /// also return additional types in the
future.
On 2014/05/08 23:56:13, Bob Nystrom wrote:
> On 2014/05/08 21:12:36, nweiz wrote:
> > On 2014/05/08 20:30:48, Bob Nystrom wrote:
> > > I know we talked about this a bit before but I don't remember the details.
> > > 
> > > Is this class just used to associate logs with a specific transformer? If
> so,
> > I
> > > think it would be good to move to a dumber object here. I worry including
> the
> > > actual object here is future-API-evolution hostile.
> > 
> > I think it's useful for users to be able to reliably check whether a log
entry
> > came from their transformer, and the best way to do that is to allow them to
> > write "myTransformer == logEntry.transform.transformer".
> 
> Maybe, but I think we could support that with myTransformer.id ==
> logEntry.transformer.id just as well.
> 
> But do what you like here.

I'm going to use the pain of backwards incompatibility as a tiebreaker and keep
it as-is.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
File pkg/barback/lib/src/graph/transform_node.dart (right):

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:571: if (_state ==
_State.NEEDS_DECLARE) return false;
On 2014/05/08 23:56:13, Bob Nystrom wrote:
> On 2014/05/08 21:12:36, nweiz wrote:
> > On 2014/05/08 20:30:48, Bob Nystrom wrote:
> > > What if _state is DECLARING? Can that still happen?
> > 
> > There's no way to transition to DECLARING until `apply` has finished
running.
> 
> Add an assert above then?

Done.

https://codereview.chromium.org/267393009/diff/1/pkg/barback/lib/src/graph/tr...
pkg/barback/lib/src/graph/transform_node.dart:571: if (_state ==
_State.NEEDS_DECLARE) return false;
On 2014/05/08 23:56:13, Bob Nystrom wrote:
> On 2014/05/08 21:12:36, nweiz wrote:
> > On 2014/05/08 20:30:48, Bob Nystrom wrote:
> > > What if _state is DECLARING? Can that still happen?
> > 
> > There's no way to transition to DECLARING until `apply` has finished
running.
> 
> Add an assert above then?

Done.

[nweiz, 2014-05-19 21:11:31]

Committed patchset #3 manually as r36325 (presubmit successful).