Only run [declareOutputs] once for each asset/transformer pair.

Only run [declareOutputs] once for each asset/transformer pair.

BUG=16442
R=rnystrom@google.com

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

[nweiz, 2014-04-08 00:13:35]

Like before, patch set 1 is the previous patch, so you only need to look at the
delta between it and patch set 2.

[Bob Nystrom, 2014-04-08 18:23:44]

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/lib/src/tran...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:110: /// This is only set if
[transformer] is a [DeclaringTransformer] and its
"set" -> "non-null".

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:364: }
You have similar code in declareOutputs. How about moving this out to a helper
method?

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
File pkg/barback/test/package_graph/declaring_transformer_test.dart (right):

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/declaring_transformer_test.dart:52: test("gets a
passed-through asset before apply is finished", () {
What if it doesn't declare that it will overwrite the asset, but then does?

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/declaring_transformer_test.dart:77: test("waits
to get an overwritten asset until apply is finished", () {
The grammar is weird here. How about "waits until apply is finished to...".

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/declaring_transformer_test.dart:133: test("can
emit outputs it didn't declare", () {
What about overwriting the primary if it didn't declare it would?

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/declaring_transformer_test.dart:141:
schedule(pumpEventQueue);
This is super sketchy.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/declaring_transformer_test.dart:146: test("can
declare outputs it doesn't emit", () {
This should at least be a warning.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
File pkg/barback/test/package_graph/lazy_transformer_test.dart (right):

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/lazy_transformer_test.dart:230: // Regression
test
"."

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/transfo...
File pkg/barback/test/transformer/declare_assets.dart (right):

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/transfo...
pkg/barback/test/transformer/declare_assets.dart:14: /// A transformer that
declares some outputs and emits others
"."

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/transfo...
pkg/barback/test/transformer/declare_assets.dart:16: final List<AssetId>
declared;
Document these.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/transfo...
File pkg/barback/test/transformer/declaring_bad.dart (right):

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/transfo...
pkg/barback/test/transformer/declaring_bad.dart:19: final bool declareError;
Document these.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/transfo...
pkg/barback/test/transformer/declaring_bad.dart:21: final String output;
How about making this an AssetId and parsing it in the constructor?

[nweiz, 2014-04-08 23:42:32]

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/lib/src/tran...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:110: /// This is only set if
[transformer] is a [DeclaringTransformer] and its
On 2014/04/08 18:23:44, Bob Nystrom wrote:
> "set" -> "non-null".

Done.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:364: }
On 2014/04/08 18:23:44, Bob Nystrom wrote:
> You have similar code in declareOutputs. How about moving this out to a helper
> method?

It's similar, but it's not identical; this filters a Set<Asset>, and
[declareOutputs] filters a Set<AssetId>.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
File pkg/barback/test/package_graph/declaring_transformer_test.dart (right):

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/declaring_transformer_test.dart:52: test("gets a
passed-through asset before apply is finished", () {
On 2014/04/08 18:23:44, Bob Nystrom wrote:
> What if it doesn't declare that it will overwrite the asset, but then does?

I think we should assume that it won't, but start returning the overwritten
asset if it does. I'll add a test for this.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/declaring_transformer_test.dart:77: test("waits
to get an overwritten asset until apply is finished", () {
On 2014/04/08 18:23:44, Bob Nystrom wrote:
> The grammar is weird here. How about "waits until apply is finished to...".

Done.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/declaring_transformer_test.dart:133: test("can
emit outputs it didn't declare", () {
On 2014/04/08 18:23:44, Bob Nystrom wrote:
> What about overwriting the primary if it didn't declare it would?

See above.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/declaring_transformer_test.dart:141:
schedule(pumpEventQueue);
On 2014/04/08 18:23:44, Bob Nystrom wrote:
> This is super sketchy.

In what way?

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/declaring_transformer_test.dart:146: test("can
declare outputs it doesn't emit", () {
On 2014/04/08 18:23:44, Bob Nystrom wrote:
> This should at least be a warning.

Done.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
File pkg/barback/test/package_graph/lazy_transformer_test.dart (right):

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/lazy_transformer_test.dart:230: // Regression
test
On 2014/04/08 18:23:44, Bob Nystrom wrote:
> "."

Done.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/transfo...
File pkg/barback/test/transformer/declare_assets.dart (right):

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/transfo...
pkg/barback/test/transformer/declare_assets.dart:14: /// A transformer that
declares some outputs and emits others
On 2014/04/08 18:23:44, Bob Nystrom wrote:
> "."

Done.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/transfo...
pkg/barback/test/transformer/declare_assets.dart:16: final List<AssetId>
declared;
On 2014/04/08 18:23:44, Bob Nystrom wrote:
> Document these.

Done.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/transfo...
File pkg/barback/test/transformer/declaring_bad.dart (right):

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/transfo...
pkg/barback/test/transformer/declaring_bad.dart:19: final bool declareError;
On 2014/04/08 18:23:44, Bob Nystrom wrote:
> Document these.

Done.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/transfo...
pkg/barback/test/transformer/declaring_bad.dart:21: final String output;
On 2014/04/08 18:23:44, Bob Nystrom wrote:
> How about making this an AssetId and parsing it in the constructor?

Done.

[Bob Nystrom, 2014-04-09 18:07:07]

LGTM, but the behavior around outputting undeclared assets still feels rough to
me.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/lib/src/tran...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:364: }
On 2014/04/08 23:42:33, nweiz wrote:
> On 2014/04/08 18:23:44, Bob Nystrom wrote:
> > You have similar code in declareOutputs. How about moving this out to a
helper
> > method?
> 
> It's similar, but it's not identical; this filters a Set<Asset>, and
> [declareOutputs] filters a Set<AssetId>.

Ah, I thought that a .map() would fix that but I guess remove/removeId gets in
the way too.

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
File pkg/barback/test/package_graph/declaring_transformer_test.dart (right):

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/declaring_transformer_test.dart:141:
schedule(pumpEventQueue);
On 2014/04/08 23:42:33, nweiz wrote:
> On 2014/04/08 18:23:44, Bob Nystrom wrote:
> > This is super sketchy.
> 
> In what way?

The comment says "probably" and then just waits an arbitrary amount of time. Is
this behavior actually deterministic?

[nweiz, 2014-04-09 20:11:52]

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
File pkg/barback/test/package_graph/declaring_transformer_test.dart (right):

https://codereview.chromium.org/227123007/diff/20001/pkg/barback/test/package...
pkg/barback/test/package_graph/declaring_transformer_test.dart:141:
schedule(pumpEventQueue);
On 2014/04/09 18:07:08, Bob Nystrom wrote:
> On 2014/04/08 23:42:33, nweiz wrote:
> > On 2014/04/08 18:23:44, Bob Nystrom wrote:
> > > This is super sketchy.
> > 
> > In what way?
> 
> The comment says "probably" and then just waits an arbitrary amount of time.
Is
> this behavior actually deterministic?

It's not arbitrary; it waits until there are no additional events in the event
queue (or as close as possible without doing zone stuff); in this case, until
the transformer is finished running. It's the same way we use [pumpEventQueue]
everywhere else.

[nweiz, 2014-04-09 20:23:57]

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