Expose aggregate transformers in barback.

Expose aggregate transformers in barback.

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

[Bob Nystrom, 2014-05-27 17:20:17]

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

https://codereview.chromium.org/299833003/diff/1/pkg/barback/lib/src/graph/ph...
pkg/barback/lib/src/graph/phase.dart:58: /// The transformer classifiers for
this phase.
Document the key type.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/transform/aggregate_test.dart (right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform/aggregate_test.dart:76: test("an
aggregate transformer is re-run if a primary input is removed", () {
What's the difference between this and the previous test?

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform/aggregate_test.dart:159:
expectNoAsset("app|foo.txt");
bar

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform/aggregate_test.dart:273: });
---

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/transform/declaring_aggregate_test.dart
(right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform/declaring_aggregate_test.dart:176:
expectAsset("app|baz.txt", "modified baz");
How does this test that the outputs are re-declared, and not just re-applied?

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/aggregate_many_to_many.dart (right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/aggregate_many_to_many.dart:15: /// given extension
and adds to their contents.
This is ambiguous. How about "all assets with a given extension, grouped by
directory".

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/aggregate_many_to_one.dart (right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/aggregate_many_to_one.dart:15: /// given extension
and concatenates them in alphabetic order by filename.
This is ambiguous (and a bit hard to read). How about:

"... applies to all assets with a given extension. For each directory containing
any of these assets, it produces an output file that contains the concatenation
of all matched assets in that directory."

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/aggregate_many_to_one.dart:20: /// The basename of
the output asset.
Mention that the output includes the directory name too.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/declaring_aggregate_many_to_many.dart (right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/declaring_aggregate_many_to_many.dart:21: Future
declareOutputs(DeclaringAggregateTransform transform) =>
Put the type parameter in the return type (if you can?).

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/mock_aggregate.dart (right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/mock_aggregate.dart:34: /// A completer for pausing
the transformer before it finishes running [apply].
Long line (probably in the mock_transformer too).

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/mock_aggregate.dart:100: /// Resumes the
transformer's [classifyPrimary] call on the asset with the given
Long line.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/mock_aggregate.dart:173: return newFuture(() {
Why is newFuture needed here?

[nweiz, 2014-05-27 21:03:10]

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

https://codereview.chromium.org/299833003/diff/1/pkg/barback/lib/src/graph/ph...
pkg/barback/lib/src/graph/phase.dart:58: /// The transformer classifiers for
this phase.
On 2014/05/27 17:20:18, Bob Nystrom wrote:
> Document the key type.

Done.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/transform/aggregate_test.dart (right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform/aggregate_test.dart:76: test("an
aggregate transformer is re-run if a primary input is removed", () {
On 2014/05/27 17:20:18, Bob Nystrom wrote:
> What's the difference between this and the previous test?

This one tests adding a new primary input. I just forgot to change the
description :p.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform/aggregate_test.dart:159:
expectNoAsset("app|foo.txt");
On 2014/05/27 17:20:18, Bob Nystrom wrote:
> bar

Done.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform/aggregate_test.dart:273: });
On 2014/05/27 17:20:18, Bob Nystrom wrote:
> ---

???

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/transform/declaring_aggregate_test.dart
(right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform/declaring_aggregate_test.dart:176:
expectAsset("app|baz.txt", "modified baz");
On 2014/05/27 17:20:18, Bob Nystrom wrote:
> How does this test that the outputs are re-declared, and not just re-applied?

Since one of its inputs is deferred, it's deferred. If it didn't re-declare the
outputs here, "baz.txt" wouldn't be declared as an output at all, and this
request would fail.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/aggregate_many_to_many.dart (right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/aggregate_many_to_many.dart:15: /// given extension
and adds to their contents.
On 2014/05/27 17:20:18, Bob Nystrom wrote:
> This is ambiguous. How about "all assets with a given extension, grouped by
> directory".

Done.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/aggregate_many_to_one.dart (right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/aggregate_many_to_one.dart:15: /// given extension
and concatenates them in alphabetic order by filename.
On 2014/05/27 17:20:18, Bob Nystrom wrote:
> This is ambiguous (and a bit hard to read). How about:
> 
> "... applies to all assets with a given extension. For each directory
containing
> any of these assets, it produces an output file that contains the
concatenation
> of all matched assets in that directory."

Done.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/aggregate_many_to_one.dart:20: /// The basename of
the output asset.
On 2014/05/27 17:20:18, Bob Nystrom wrote:
> Mention that the output includes the directory name too.

Done.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/declaring_aggregate_many_to_many.dart (right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/declaring_aggregate_many_to_many.dart:21: Future
declareOutputs(DeclaringAggregateTransform transform) =>
On 2014/05/27 17:20:18, Bob Nystrom wrote:
> Put the type parameter in the return type (if you can?).

I don't want to do that, because the specific return value isn't relevant to the
function's signature. The only thing that's relevant to the caller is that the
future will complete when the function is done declaring its outputs.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/mock_aggregate.dart (right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/mock_aggregate.dart:34: /// A completer for pausing
the transformer before it finishes running [apply].
On 2014/05/27 17:20:18, Bob Nystrom wrote:
> Long line (probably in the mock_transformer too).

Done.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/mock_aggregate.dart:100: /// Resumes the
transformer's [classifyPrimary] call on the asset with the given
On 2014/05/27 17:20:18, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/mock_aggregate.dart:173: return newFuture(() {
On 2014/05/27 17:20:18, Bob Nystrom wrote:
> Why is newFuture needed here?

We need to return "result" afterwards, and it would be out-of-scope if we just
returned directly here.

[Bob Nystrom, 2014-05-28 21:03:58]

One minor request, but LGTM!

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/transform/aggregate_test.dart (right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform/aggregate_test.dart:273: });
On 2014/05/27 21:03:10, nweiz wrote:
> On 2014/05/27 17:20:18, Bob Nystrom wrote:
> > ---
> 
> ???

Heh, oops. Left that as a note to myself to track how far I'd reviewed. :)
Disregard.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/transform/declaring_aggregate_test.dart
(right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform/declaring_aggregate_test.dart:176:
expectAsset("app|baz.txt", "modified baz");
On 2014/05/27 21:03:10, nweiz wrote:
> On 2014/05/27 17:20:18, Bob Nystrom wrote:
> > How does this test that the outputs are re-declared, and not just
re-applied?
> 
> Since one of its inputs is deferred, it's deferred. If it didn't re-declare
the
> outputs here, "baz.txt" wouldn't be declared as an output at all, and this
> request would fail.

Ah, so this relies on requests for undeclared outputs not triggering a build.
But if, for some reason, that transformer got tickled into running, baz.txt
would appear as an output, right?

How about a little comment explaining this?

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/declaring_aggregate_many_to_many.dart (right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/declaring_aggregate_many_to_many.dart:21: Future
declareOutputs(DeclaringAggregateTransform transform) =>
On 2014/05/27 21:03:10, nweiz wrote:
> On 2014/05/27 17:20:18, Bob Nystrom wrote:
> > Put the type parameter in the return type (if you can?).
> 
> I don't want to do that, because the specific return value isn't relevant to
the
> function's signature. The only thing that's relevant to the caller is that the
> future will complete when the function is done declaring its outputs.

Okey-dokey.

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/mock_aggregate.dart (right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/mock_aggregate.dart:173: return newFuture(() {
On 2014/05/27 21:03:10, nweiz wrote:
> On 2014/05/27 17:20:18, Bob Nystrom wrote:
> > Why is newFuture needed here?
> 
> We need to return "result" afterwards, and it would be out-of-scope if we just
> returned directly here.

Oh, right. Thanks for clarifying.

[nweiz, 2014-05-28 23:32:11]

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/transform/declaring_aggregate_test.dart
(right):

https://codereview.chromium.org/299833003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform/declaring_aggregate_test.dart:176:
expectAsset("app|baz.txt", "modified baz");
On 2014/05/28 21:03:58, Bob Nystrom wrote:
> On 2014/05/27 21:03:10, nweiz wrote:
> > On 2014/05/27 17:20:18, Bob Nystrom wrote:
> > > How does this test that the outputs are re-declared, and not just
> re-applied?
> > 
> > Since one of its inputs is deferred, it's deferred. If it didn't re-declare
> the
> > outputs here, "baz.txt" wouldn't be declared as an output at all, and this
> > request would fail.
> 
> Ah, so this relies on requests for undeclared outputs not triggering a build.
> But if, for some reason, that transformer got tickled into running, baz.txt
> would appear as an output, right?
> 
> How about a little comment explaining this?

Done.

[nweiz, 2014-05-28 23:33:46]

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