Barback transforms now pass through the primary input by default.

Barback transforms now pass through the primary input by default.

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

[Bob Nystrom, 2014-02-26 01:09:15]

https://codereview.chromium.org/180473003/diff/1/pkg/barback/lib/src/phase_fo...
File pkg/barback/lib/src/phase_forwarder.dart (right):

https://codereview.chromium.org/180473003/diff/1/pkg/barback/lib/src/phase_fo...
pkg/barback/lib/src/phase_forwarder.dart:73: asset.onStateChange.listen((state)
=> _adjustOutput());
state -> _

https://codereview.chromium.org/180473003/diff/1/pkg/barback/lib/src/phase_in...
File pkg/barback/lib/src/phase_input.dart (right):

https://codereview.chromium.org/180473003/diff/1/pkg/barback/lib/src/phase_in...
pkg/barback/lib/src/phase_input.dart:54: /// removed if they do. It's null if
the asset is not being passed through.
do -> will

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/add_remove_transform_test.dart (left):

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/add_remove_transform_test.dart:23:
expectNoAsset("app|foo.blub");
How about changing these to positive expectations instead of deleting them?

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

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/add_remove_transform_test.dart:260: [rewrite, new
RewriteTransformer("txt", "txt")]
I'm not clear on what's going on here. Are you trying to show that a new
transformer will see an asset previously passed-through by an existing
transformer?

If so, shouldn't it be in a later phase?

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

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/group_test.dart:24: expectAsset("app|foo.b",
"foo.b");
Why keep this one but delete the others?

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/group_test.dart:261: "by transformers", () {
It does pass it through now.

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/group_test.dart:274: test("doesn't pass through
an input that's overwritten by transformers but "
Does.

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

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform_test.dart:888: test("doesn't pass an
asset through a phase in which a transform overwrites "
Long line.

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform_test.dart:1273: });
This file is a beast. In a separate patch, how about moving these groups to
separate files?

[nweiz, 2014-02-26 19:37:14]

https://codereview.chromium.org/180473003/diff/1/pkg/barback/lib/src/phase_fo...
File pkg/barback/lib/src/phase_forwarder.dart (right):

https://codereview.chromium.org/180473003/diff/1/pkg/barback/lib/src/phase_fo...
pkg/barback/lib/src/phase_forwarder.dart:73: asset.onStateChange.listen((state)
=> _adjustOutput());
On 2014/02/26 01:09:15, Bob Nystrom wrote:
> state -> _

Done.

https://codereview.chromium.org/180473003/diff/1/pkg/barback/lib/src/phase_in...
File pkg/barback/lib/src/phase_input.dart (right):

https://codereview.chromium.org/180473003/diff/1/pkg/barback/lib/src/phase_in...
pkg/barback/lib/src/phase_input.dart:54: /// removed if they do. It's null if
the asset is not being passed through.
On 2014/02/26 01:09:15, Bob Nystrom wrote:
> do -> will

I don't think that's right -- we don't emit the pass-through when transforms
will overwrite the primary input in the future, we emit it when they actually do
overwrite it in the present.

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/add_remove_transform_test.dart (left):

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/add_remove_transform_test.dart:23:
expectNoAsset("app|foo.blub");
On 2014/02/26 01:09:15, Bob Nystrom wrote:
> How about changing these to positive expectations instead of deleting them?

They're not really relevant to the tests they're in. The negative assertions
were relevant, in that they were another way of saying "the transformer we
expected to run did", but the positive assertions would just be duplicating the
explicit pass-through tests.

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

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/add_remove_transform_test.dart:260: [rewrite, new
RewriteTransformer("txt", "txt")]
On 2014/02/26 01:09:15, Bob Nystrom wrote:
> I'm not clear on what's going on here. Are you trying to show that a new
> transformer will see an asset previously passed-through by an existing
> transformer?
> 
> If so, shouldn't it be in a later phase?

I'm asserting that a transform that's added to an existing phase can overwrite
an existing asset in that phase.

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

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/group_test.dart:24: expectAsset("app|foo.b",
"foo.b");
On 2014/02/26 01:09:15, Bob Nystrom wrote:
> Why keep this one but delete the others?

I want to assert that a pass-through asset makes it out of the group.

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/group_test.dart:261: "by transformers", () {
On 2014/02/26 01:09:15, Bob Nystrom wrote:
> It does pass it through now.

No, the output is "foo.a", which is the overwritten value. The original value is
just "foo".

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/group_test.dart:274: test("doesn't pass through
an input that's overwritten by transformers but "
On 2014/02/26 01:09:15, Bob Nystrom wrote:
> Does.

See above.

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

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform_test.dart:888: test("doesn't pass an
asset through a phase in which a transform overwrites "
On 2014/02/26 01:09:15, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/transform_test.dart:1273: });
On 2014/02/26 01:09:15, Bob Nystrom wrote:
> This file is a beast. In a separate patch, how about moving these groups to
> separate files?

Sure.

[Bob Nystrom, 2014-02-26 20:28:49]

https://codereview.chromium.org/180473003/diff/1/pkg/barback/lib/src/phase_in...
File pkg/barback/lib/src/phase_input.dart (right):

https://codereview.chromium.org/180473003/diff/1/pkg/barback/lib/src/phase_in...
pkg/barback/lib/src/phase_input.dart:54: /// removed if they do. It's null if
the asset is not being passed through.
On 2014/02/26 19:37:14, nweiz wrote:
> On 2014/02/26 01:09:15, Bob Nystrom wrote:
> > do -> will
> 
> I don't think that's right -- we don't emit the pass-through when transforms
> will overwrite the primary input in the future, we emit it when they actually
do
> overwrite it in the present.

Oh, now I get it. That sentence is hairy. "removed" -> "be marked removed".

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

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/add_remove_transform_test.dart:260: [rewrite, new
RewriteTransformer("txt", "txt")]
On 2014/02/26 19:37:14, nweiz wrote:
> On 2014/02/26 01:09:15, Bob Nystrom wrote:
> > I'm not clear on what's going on here. Are you trying to show that a new
> > transformer will see an asset previously passed-through by an existing
> > transformer?
> > 
> > If so, shouldn't it be in a later phase?
> 
> I'm asserting that a transform that's added to an existing phase can overwrite
> an existing asset in that phase.

OK, add some docs to clarify.

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

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/group_test.dart:24: expectAsset("app|foo.b",
"foo.b");
On 2014/02/26 19:37:14, nweiz wrote:
> On 2014/02/26 01:09:15, Bob Nystrom wrote:
> > Why keep this one but delete the others?
> 
> I want to assert that a pass-through asset makes it out of the group.

That's not what this test description is about. If that's what you want to test
document that or make a separate test.

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/group_test.dart:261: "by transformers", () {
On 2014/02/26 19:37:14, nweiz wrote:
> On 2014/02/26 01:09:15, Bob Nystrom wrote:
> > It does pass it through now.
> 
> No, the output is "foo.a", which is the overwritten value. The original value
is
> just "foo".

I guess I just don't understand the utility of this test. How could the input
possibly be passed through if it's being overwritten?

[nweiz, 2014-02-27 21:04:19]

https://codereview.chromium.org/180473003/diff/1/pkg/barback/lib/src/phase_in...
File pkg/barback/lib/src/phase_input.dart (right):

https://codereview.chromium.org/180473003/diff/1/pkg/barback/lib/src/phase_in...
pkg/barback/lib/src/phase_input.dart:54: /// removed if they do. It's null if
the asset is not being passed through.
On 2014/02/26 20:28:50, Bob Nystrom wrote:
> On 2014/02/26 19:37:14, nweiz wrote:
> > On 2014/02/26 01:09:15, Bob Nystrom wrote:
> > > do -> will
> > 
> > I don't think that's right -- we don't emit the pass-through when transforms
> > will overwrite the primary input in the future, we emit it when they
actually
> do
> > overwrite it in the present.
> 
> Oh, now I get it. That sentence is hairy. "removed" -> "be marked removed".

Done.

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

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/add_remove_transform_test.dart:260: [rewrite, new
RewriteTransformer("txt", "txt")]
On 2014/02/26 20:28:50, Bob Nystrom wrote:
> On 2014/02/26 19:37:14, nweiz wrote:
> > On 2014/02/26 01:09:15, Bob Nystrom wrote:
> > > I'm not clear on what's going on here. Are you trying to show that a new
> > > transformer will see an asset previously passed-through by an existing
> > > transformer?
> > > 
> > > If so, shouldn't it be in a later phase?
> > 
> > I'm asserting that a transform that's added to an existing phase can
overwrite
> > an existing asset in that phase.
> 
> OK, add some docs to clarify.

Done.

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

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/group_test.dart:24: expectAsset("app|foo.b",
"foo.b");
On 2014/02/26 20:28:50, Bob Nystrom wrote:
> On 2014/02/26 19:37:14, nweiz wrote:
> > On 2014/02/26 01:09:15, Bob Nystrom wrote:
> > > Why keep this one but delete the others?
> > 
> > I want to assert that a pass-through asset makes it out of the group.
> 
> That's not what this test description is about. If that's what you want to
test
> document that or make a separate test.

Done.

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/group_test.dart:261: "by transformers", () {
On 2014/02/26 20:28:50, Bob Nystrom wrote:
> On 2014/02/26 19:37:14, nweiz wrote:
> > On 2014/02/26 01:09:15, Bob Nystrom wrote:
> > > It does pass it through now.
> > 
> > No, the output is "foo.a", which is the overwritten value. The original
value
> is
> > just "foo".
> 
> I guess I just don't understand the utility of this test. How could the input
> possibly be passed through if it's being overwritten?

It shouldn't be able to; if it did, it might cause a collision or the resulting
"foo.a" asset might just silently be the source asset rather than the
overwritten asset. This test validates that neither of those happen.

[Bob Nystrom, 2014-02-27 21:33:40]

LGTM!

Do you plan to add support for explicitly consuming the primary soon?

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

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/group_test.dart:261: "by transformers", () {
On 2014/02/27 21:04:20, nweiz wrote:
> On 2014/02/26 20:28:50, Bob Nystrom wrote:
> > On 2014/02/26 19:37:14, nweiz wrote:
> > > On 2014/02/26 01:09:15, Bob Nystrom wrote:
> > > > It does pass it through now.
> > > 
> > > No, the output is "foo.a", which is the overwritten value. The original
> value
> > is
> > > just "foo".
> > 
> > I guess I just don't understand the utility of this test. How could the
input
> > possibly be passed through if it's being overwritten?
> 
> It shouldn't be able to; if it did, it might cause a collision or the
resulting
> "foo.a" asset might just silently be the source asset rather than the
> overwritten asset. This test validates that neither of those happen.

SGTM, though a comment to this affect would be nice too.

[nweiz, 2014-02-27 21:47:56]

I have some queued CLs that culminate in adding [consumePrimary].

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

https://codereview.chromium.org/180473003/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/group_test.dart:261: "by transformers", () {
On 2014/02/27 21:33:40, Bob Nystrom wrote:
> On 2014/02/27 21:04:20, nweiz wrote:
> > On 2014/02/26 20:28:50, Bob Nystrom wrote:
> > > On 2014/02/26 19:37:14, nweiz wrote:
> > > > On 2014/02/26 01:09:15, Bob Nystrom wrote:
> > > > > It does pass it through now.
> > > > 
> > > > No, the output is "foo.a", which is the overwritten value. The original
> > value
> > > is
> > > > just "foo".
> > > 
> > > I guess I just don't understand the utility of this test. How could the
> input
> > > possibly be passed through if it's being overwritten?
> > 
> > It shouldn't be able to; if it did, it might cause a collision or the
> resulting
> > "foo.a" asset might just silently be the source asset rather than the
> > overwritten asset. This test validates that neither of those happen.
> 
> SGTM, though a comment to this affect would be nice too.

I'll add one to the simplest/most general test case for this stuff.

[nweiz, 2014-02-27 21:47:59]

I have some queued CLs that culminate in adding [consumePrimary].

[nweiz, 2014-02-27 21:49:25]

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