Make barback more package-aware.

Make barback more package-aware.

This makes AssetGraph package-specific, but doesn't yet support cross-package asset usage.

R=rnystrom@google.com

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

[Bob Nystrom, 2013-07-11 23:03:40]

Needs some more tests. Offhand:

1. Test that transformers are only applied to sources in the package they are
associated with.
2. Ditto for generated assets.

What happens if a transformer generates an asset in a different package?

https://codereview.chromium.org/5695057915019264/diff/5629499534213120/pkg/ba...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/5695057915019264/diff/5629499534213120/pkg/ba...
pkg/barback/lib/src/asset_graph.dart:139: assert(sources.every((id) =>
id.package == package));
Alternatively, it could just ignore sources outside of its package. That might
be easier on the caller.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
File pkg/barback/lib/src/asset_graph_manager.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_graph_manager.dart:18: // TODO(nweiz): come up with
better names for this and AssetGraph.
Agreed. "Manager" is one of my biggest pet peeves.

How about "PackageGraph"?

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_graph_manager.dart:31: /// The current [BuildResult]
for each package's [AssetGraph].
"current" -> "last"

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_graph_manager.dart:63: this, package,
provider.getTransformers(package));
Nit, but wrapping here looks weird to me. How about moving "this, package," to
the previous line?

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_graph_manager.dart:64: _graphResults[package] = new
BuildResult.success();
Why is this initially success and not null? Document.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_graph_manager.dart:71: if
(_graphResults.values.any((result) => result == null)) return;
Having to walk this every time a package completes seems a bit gross. Here's a
thought:

Maybe AssetGraph should send an event when a build starts and then when it
completes. Then this class could keep a running tally of the ongoing builds.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
File pkg/barback/lib/src/asset_provider.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_provider.dart:36: Iterable<Iterable<Transformer>>
getTransformers(String package);
Putting Transformers in here makes me think we should change the name of this
class. I'm not sure what to, but "AssetProvider" isn't accurate anymore.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
File pkg/barback/lib/src/utils.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/utils.dart:38: if (element is List) {
is Iterable

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/utils.dart:58: var controller = new StreamController(sync:
true);
Document why "sync: true"

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
File pkg/barback/test/utils.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
pkg/barback/test/utils.dart:115: /// Asserts that the build should not terminate
at this point in the schedule.
"should not terminate at this point" is unclear. Does this assert that the build
has not terminated by the time this has called?

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
pkg/barback/test/utils.dart:135: return pumpEventQueue().then((_) {
Explain/document this.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
pkg/barback/test/utils.dart:264: Map<String, Iterable<Iterable<Transformer>>>
transformers) {
How about also allowing this to just be a nested list of transformers, and
assuming a single "app" package in that case?

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
pkg/barback/test/utils.dart:332: class _MockPackage {
Document.

[nweiz, 2013-07-15 22:11:44]

I've added tests for emitting a cross-package asset and for transformers only
being applied in a given package. I'm not sure what you mean by testing for
generated assets.

https://codereview.chromium.org/5695057915019264/diff/5629499534213120/pkg/ba...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/5695057915019264/diff/5629499534213120/pkg/ba...
pkg/barback/lib/src/asset_graph.dart:139: assert(sources.every((id) =>
id.package == package));
On 2013/07/11 23:03:40, Bob Nystrom wrote:
> Alternatively, it could just ignore sources outside of its package. That might
> be easier on the caller.

Since this should only be called by [AssetGraphManager], I think it makes sense
to be strict about what gets passed in. If we're getting an id from a different
package, something's probably seriously wrong.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
File pkg/barback/lib/src/asset_graph_manager.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_graph_manager.dart:18: // TODO(nweiz): come up with
better names for this and AssetGraph.
On 2013/07/11 23:03:40, Bob Nystrom wrote:
> Agreed. "Manager" is one of my biggest pet peeves.
> 
> How about "PackageGraph"?

I like "PackageGraph", but where does that leave AssetGraph? I don't want to
just call it "Package", but it won't really encapsulate an asset graph once
transforms start reaching outside a given AssetGraph for cross-package assets.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_graph_manager.dart:31: /// The current [BuildResult]
for each package's [AssetGraph].
On 2013/07/11 23:03:40, Bob Nystrom wrote:
> "current" -> "last"

I think "current" is slightly more accurate than "last". "last" implies that it
will exist until the next [BuildResult] exists, where in fact it will stop
existing once something changes in the [AssetGraph].

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_graph_manager.dart:63: this, package,
provider.getTransformers(package));
On 2013/07/11 23:03:40, Bob Nystrom wrote:
> Nit, but wrapping here looks weird to me. How about moving "this, package," to
> the previous line?

Done.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_graph_manager.dart:64: _graphResults[package] = new
BuildResult.success();
On 2013/07/11 23:03:40, Bob Nystrom wrote:
> Why is this initially success and not null?

I wanted to make it consistent with the fact that the assets don't start
building until [updateSources] is called. At some point I'd like to add a more
principled way of kicking off a build, but for now I think keeping these
consistent makes sense.

> Document.

Done.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_graph_manager.dart:71: if
(_graphResults.values.any((result) => result == null)) return;
On 2013/07/11 23:03:40, Bob Nystrom wrote:
> Having to walk this every time a package completes seems a bit gross. Here's a
> thought:
> 
> Maybe AssetGraph should send an event when a build starts and then when it
> completes. Then this class could keep a running tally of the ongoing builds.

I don't think there are likely to be enough packages in the graph to make this
slow, especially since it's naturally limited by how often the user modifies
files. I don't think it's worth the complexity of adding a new event type to
[AssetGraph].

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
File pkg/barback/lib/src/asset_provider.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_provider.dart:36: Iterable<Iterable<Transformer>>
getTransformers(String package);
On 2013/07/11 23:03:40, Bob Nystrom wrote:
> Putting Transformers in here makes me think we should change the name of this
> class. I'm not sure what to, but "AssetProvider" isn't accurate anymore.

Added a TODO. I'll do a name-change-only CL after this one.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
File pkg/barback/lib/src/utils.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/utils.dart:38: if (element is List) {
On 2013/07/11 23:03:40, Bob Nystrom wrote:
> is Iterable

That seems a little dangerous. A lot of things can extend Iterable that may not
want to be flattened.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/utils.dart:58: var controller = new StreamController(sync:
true);
On 2013/07/11 23:03:40, Bob Nystrom wrote:
> Document why "sync: true"

Done.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
File pkg/barback/test/utils.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
pkg/barback/test/utils.dart:115: /// Asserts that the build should not terminate
at this point in the schedule.
On 2013/07/11 23:03:40, Bob Nystrom wrote:
> "should not terminate at this point" is unclear. Does this assert that the
build
> has not terminated by the time this has called?

Clarified.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
pkg/barback/test/utils.dart:135: return pumpEventQueue().then((_) {
On 2013/07/11 23:03:40, Bob Nystrom wrote:
> Explain/document this.

Done.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
pkg/barback/test/utils.dart:264: Map<String, Iterable<Iterable<Transformer>>>
transformers) {
On 2013/07/11 23:03:40, Bob Nystrom wrote:
> How about also allowing this to just be a nested list of transformers, and
> assuming a single "app" package in that case?

I thought about that, but it seemed weird to assume a default package name when
nothing else in the framework did. It felt weird to write "app|foo.txt" for all
the ids but then not write "app" in the transformers.

If you think removing some test boilerplate is worthwhile, I could make "app"
the default in both cases.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
pkg/barback/test/utils.dart:332: class _MockPackage {
On 2013/07/11 23:03:40, Bob Nystrom wrote:
> Document.

Done.

[Bob Nystrom, 2013-07-15 23:25:25]

On 2013/07/15 22:11:44, nweiz wrote:
> I've added tests for emitting a cross-package asset and for transformers only
> being applied in a given package. I'm not sure what you mean by testing for
> generated assets.
> 
>
https://codereview.chromium.org/5695057915019264/diff/5629499534213120/pkg/ba...
> File pkg/barback/lib/src/asset_graph.dart (right):
> 
>
https://codereview.chromium.org/5695057915019264/diff/5629499534213120/pkg/ba...
> pkg/barback/lib/src/asset_graph.dart:139: assert(sources.every((id) =>
> id.package == package));
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > Alternatively, it could just ignore sources outside of its package. That
might
> > be easier on the caller.
> 
> Since this should only be called by [AssetGraphManager], I think it makes
sense
> to be strict about what gets passed in. If we're getting an id from a
different
> package, something's probably seriously wrong.
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> File pkg/barback/lib/src/asset_graph_manager.dart (right):
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> pkg/barback/lib/src/asset_graph_manager.dart:18: // TODO(nweiz): come up with
> better names for this and AssetGraph.
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > Agreed. "Manager" is one of my biggest pet peeves.
> > 
> > How about "PackageGraph"?
> 
> I like "PackageGraph", but where does that leave AssetGraph? I don't want to
> just call it "Package", but it won't really encapsulate an asset graph once
> transforms start reaching outside a given AssetGraph for cross-package assets.
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> pkg/barback/lib/src/asset_graph_manager.dart:31: /// The current [BuildResult]
> for each package's [AssetGraph].
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > "current" -> "last"
> 
> I think "current" is slightly more accurate than "last". "last" implies that
it
> will exist until the next [BuildResult] exists, where in fact it will stop
> existing once something changes in the [AssetGraph].
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> pkg/barback/lib/src/asset_graph_manager.dart:63: this, package,
> provider.getTransformers(package));
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > Nit, but wrapping here looks weird to me. How about moving "this, package,"
to
> > the previous line?
> 
> Done.
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> pkg/barback/lib/src/asset_graph_manager.dart:64: _graphResults[package] = new
> BuildResult.success();
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > Why is this initially success and not null?
> 
> I wanted to make it consistent with the fact that the assets don't start
> building until [updateSources] is called. At some point I'd like to add a more
> principled way of kicking off a build, but for now I think keeping these
> consistent makes sense.
> 
> > Document.
> 
> Done.
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> pkg/barback/lib/src/asset_graph_manager.dart:71: if
> (_graphResults.values.any((result) => result == null)) return;
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > Having to walk this every time a package completes seems a bit gross. Here's
a
> > thought:
> > 
> > Maybe AssetGraph should send an event when a build starts and then when it
> > completes. Then this class could keep a running tally of the ongoing builds.
> 
> I don't think there are likely to be enough packages in the graph to make this
> slow, especially since it's naturally limited by how often the user modifies
> files. I don't think it's worth the complexity of adding a new event type to
> [AssetGraph].
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> File pkg/barback/lib/src/asset_provider.dart (right):
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> pkg/barback/lib/src/asset_provider.dart:36: Iterable<Iterable<Transformer>>
> getTransformers(String package);
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > Putting Transformers in here makes me think we should change the name of
this
> > class. I'm not sure what to, but "AssetProvider" isn't accurate anymore.
> 
> Added a TODO. I'll do a name-change-only CL after this one.
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> File pkg/barback/lib/src/utils.dart (right):
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> pkg/barback/lib/src/utils.dart:38: if (element is List) {
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > is Iterable
> 
> That seems a little dangerous. A lot of things can extend Iterable that may
not
> want to be flattened.
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> pkg/barback/lib/src/utils.dart:58: var controller = new StreamController(sync:
> true);
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > Document why "sync: true"
> 
> Done.
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
> File pkg/barback/test/utils.dart (right):
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
> pkg/barback/test/utils.dart:115: /// Asserts that the build should not
terminate
> at this point in the schedule.
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > "should not terminate at this point" is unclear. Does this assert that the
> build
> > has not terminated by the time this has called?
> 
> Clarified.
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
> pkg/barback/test/utils.dart:135: return pumpEventQueue().then((_) {
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > Explain/document this.
> 
> Done.
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
> pkg/barback/test/utils.dart:264: Map<String, Iterable<Iterable<Transformer>>>
> transformers) {
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > How about also allowing this to just be a nested list of transformers, and
> > assuming a single "app" package in that case?
> 
> I thought about that, but it seemed weird to assume a default package name
when
> nothing else in the framework did. It felt weird to write "app|foo.txt" for
all
> the ids but then not write "app" in the transformers.
> 
> If you think removing some test boilerplate is worthwhile, I could make "app"
> the default in both cases.
> 
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
> pkg/barback/test/utils.dart:332: class _MockPackage {
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > Document.
> 
> Done.

> I'm not sure what you mean by testing for generated assets.

It might be good to have a test for:

1. You have two packages A and B.
2. A has a transformer that takes ".foo" files and outputs ".bar".
3. B has a transformer that takes ".bar" files and outputs ".baz".
4. There is a .foo file in A.
5. That should *not* result in any .baz files being generated.

I think we test that we won't look for primary inputs in *sources* from other
packages, but we should also test that we don't look for primary inputs in
*generated* assets in other packages.

[nweiz, 2013-07-16 00:13:42]

On 2013/07/15 23:25:25, Bob Nystrom wrote:
> On 2013/07/15 22:11:44, nweiz wrote:
> > I've added tests for emitting a cross-package asset and for transformers
only
> > being applied in a given package. I'm not sure what you mean by testing for
> > generated assets.
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/5629499534213120/pkg/ba...
> > File pkg/barback/lib/src/asset_graph.dart (right):
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/5629499534213120/pkg/ba...
> > pkg/barback/lib/src/asset_graph.dart:139: assert(sources.every((id) =>
> > id.package == package));
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > Alternatively, it could just ignore sources outside of its package. That
> might
> > > be easier on the caller.
> > 
> > Since this should only be called by [AssetGraphManager], I think it makes
> sense
> > to be strict about what gets passed in. If we're getting an id from a
> different
> > package, something's probably seriously wrong.
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> > File pkg/barback/lib/src/asset_graph_manager.dart (right):
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> > pkg/barback/lib/src/asset_graph_manager.dart:18: // TODO(nweiz): come up
with
> > better names for this and AssetGraph.
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > Agreed. "Manager" is one of my biggest pet peeves.
> > > 
> > > How about "PackageGraph"?
> > 
> > I like "PackageGraph", but where does that leave AssetGraph? I don't want to
> > just call it "Package", but it won't really encapsulate an asset graph once
> > transforms start reaching outside a given AssetGraph for cross-package
assets.
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> > pkg/barback/lib/src/asset_graph_manager.dart:31: /// The current
[BuildResult]
> > for each package's [AssetGraph].
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > "current" -> "last"
> > 
> > I think "current" is slightly more accurate than "last". "last" implies that
> it
> > will exist until the next [BuildResult] exists, where in fact it will stop
> > existing once something changes in the [AssetGraph].
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> > pkg/barback/lib/src/asset_graph_manager.dart:63: this, package,
> > provider.getTransformers(package));
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > Nit, but wrapping here looks weird to me. How about moving "this,
package,"
> to
> > > the previous line?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> > pkg/barback/lib/src/asset_graph_manager.dart:64: _graphResults[package] =
new
> > BuildResult.success();
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > Why is this initially success and not null?
> > 
> > I wanted to make it consistent with the fact that the assets don't start
> > building until [updateSources] is called. At some point I'd like to add a
more
> > principled way of kicking off a build, but for now I think keeping these
> > consistent makes sense.
> > 
> > > Document.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> > pkg/barback/lib/src/asset_graph_manager.dart:71: if
> > (_graphResults.values.any((result) => result == null)) return;
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > Having to walk this every time a package completes seems a bit gross.
Here's
> a
> > > thought:
> > > 
> > > Maybe AssetGraph should send an event when a build starts and then when it
> > > completes. Then this class could keep a running tally of the ongoing
builds.
> > 
> > I don't think there are likely to be enough packages in the graph to make
this
> > slow, especially since it's naturally limited by how often the user modifies
> > files. I don't think it's worth the complexity of adding a new event type to
> > [AssetGraph].
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> > File pkg/barback/lib/src/asset_provider.dart (right):
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> > pkg/barback/lib/src/asset_provider.dart:36: Iterable<Iterable<Transformer>>
> > getTransformers(String package);
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > Putting Transformers in here makes me think we should change the name of
> this
> > > class. I'm not sure what to, but "AssetProvider" isn't accurate anymore.
> > 
> > Added a TODO. I'll do a name-change-only CL after this one.
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> > File pkg/barback/lib/src/utils.dart (right):
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> > pkg/barback/lib/src/utils.dart:38: if (element is List) {
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > is Iterable
> > 
> > That seems a little dangerous. A lot of things can extend Iterable that may
> not
> > want to be flattened.
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
> > pkg/barback/lib/src/utils.dart:58: var controller = new
StreamController(sync:
> > true);
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > Document why "sync: true"
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
> > File pkg/barback/test/utils.dart (right):
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
> > pkg/barback/test/utils.dart:115: /// Asserts that the build should not
> terminate
> > at this point in the schedule.
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > "should not terminate at this point" is unclear. Does this assert that the
> > build
> > > has not terminated by the time this has called?
> > 
> > Clarified.
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
> > pkg/barback/test/utils.dart:135: return pumpEventQueue().then((_) {
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > Explain/document this.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
> > pkg/barback/test/utils.dart:264: Map<String,
Iterable<Iterable<Transformer>>>
> > transformers) {
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > How about also allowing this to just be a nested list of transformers, and
> > > assuming a single "app" package in that case?
> > 
> > I thought about that, but it seemed weird to assume a default package name
> when
> > nothing else in the framework did. It felt weird to write "app|foo.txt" for
> all
> > the ids but then not write "app" in the transformers.
> > 
> > If you think removing some test boilerplate is worthwhile, I could make
"app"
> > the default in both cases.
> > 
> >
>
https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
> > pkg/barback/test/utils.dart:332: class _MockPackage {
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > Document.
> > 
> > Done.
> 
> > I'm not sure what you mean by testing for generated assets.
> 
> It might be good to have a test for:
> 
> 1. You have two packages A and B.
> 2. A has a transformer that takes ".foo" files and outputs ".bar".
> 3. B has a transformer that takes ".bar" files and outputs ".baz".
> 4. There is a .foo file in A.
> 5. That should *not* result in any .baz files being generated.
> 
> I think we test that we won't look for primary inputs in *sources* from other
> packages, but we should also test that we don't look for primary inputs in
> *generated* assets in other packages.

Done.

[Bob Nystrom, 2013-07-16 17:36:01]

I thought these comments were sent, but I guess I just accidentally replied to
the post without sending them. Sorry!

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
File pkg/barback/lib/src/asset_graph_manager.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_graph_manager.dart:18: // TODO(nweiz): come up with
better names for this and AssetGraph.
On 2013/07/15 22:11:44, nweiz wrote:
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > Agreed. "Manager" is one of my biggest pet peeves.
> > 
> > How about "PackageGraph"?
> 
> I like "PackageGraph", but where does that leave AssetGraph? I don't want to
> just call it "Package", but it won't really encapsulate an asset graph once
> transforms start reaching outside a given AssetGraph for cross-package assets.

How about:

AssetGraphManager -> AssetGraph
AssetGraph -> PackageAssetGraph

?

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
File pkg/barback/lib/src/utils.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/utils.dart:38: if (element is List) {
On 2013/07/15 22:11:44, nweiz wrote:
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > is Iterable
> 
> That seems a little dangerous. A lot of things can extend Iterable that may
not
> want to be flattened.

Conversely, a lot of things extend iterable that do want to be flattened. I
would expect [foo.where(...)] to flatten the results of that .where() call but
this won't do that.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
File pkg/barback/test/utils.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
pkg/barback/test/utils.dart:264: Map<String, Iterable<Iterable<Transformer>>>
transformers) {
On 2013/07/15 22:11:44, nweiz wrote:
> On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > How about also allowing this to just be a nested list of transformers, and
> > assuming a single "app" package in that case?
> 
> I thought about that, but it seemed weird to assume a default package name
when
> nothing else in the framework did. It felt weird to write "app|foo.txt" for
all
> the ids but then not write "app" in the transformers.
> 
> If you think removing some test boilerplate is worthwhile, I could make "app"
> the default in both cases.

Yeah, I was actually thinking about doing the former too but didn't get around
to it. I'd be fine either way.

https://codereview.chromium.org/5695057915019264/diff/14001/pkg/barback/lib/s...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/5695057915019264/diff/14001/pkg/barback/lib/s...
pkg/barback/lib/src/transform_node.dart:94: // have failed?
Probably not, I think.

https://codereview.chromium.org/5695057915019264/diff/14001/pkg/barback/test/...
File pkg/barback/test/utils.dart (right):

https://codereview.chromium.org/5695057915019264/diff/14001/pkg/barback/test/...
pkg/barback/test/utils.dart:345: /// A struct used by [MockProvider] to keep
track of which assets and
"struct" is out of place here. How about just "Used by [MockProvider] to ..."

https://codereview.chromium.org/5695057915019264/diff/14001/pkg/barback/test/...
pkg/barback/test/utils.dart:534: /// A transformer that outputs an asset with
the wrong package name.
That may be how it's used, but it isn't what it does. What it does is take in
any input as a primary and always outputs a fixed output with a given ID. Name
and document accordingly.

[nweiz, 2013-07-16 19:39:39]

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
File pkg/barback/lib/src/asset_graph_manager.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_graph_manager.dart:18: // TODO(nweiz): come up with
better names for this and AssetGraph.
 2013/07/16 17:36:01, Bob Nystrom wrote:
> On 2013/07/15 22:11:44, nweiz wrote:
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > Agreed. "Manager" is one of my biggest pet peeves.
> > > 
> > > How about "PackageGraph"?
> > 
> > I like "PackageGraph", but where does that leave AssetGraph? I don't want to
> > just call it "Package", but it won't really encapsulate an asset graph once
> > transforms start reaching outside a given AssetGraph for cross-package
assets.
> 
> How about:
> 
> AssetGraphManager -> AssetGraph
> AssetGraph -> PackageAssetGraph
> 
> ?

I was thinking

AssetGraphManager -> PackageGraph
AssetGraph        -> AssetCascade

With "cascade" representing the way assets cascade through a series of phases.
This has the side benefit of having a different noun for the two classes, so a
variable named "_graph" consistently refers to a single type.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
File pkg/barback/lib/src/utils.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/utils.dart:38: if (element is List) {
On 2013/07/16 17:36:01, Bob Nystrom wrote:
> On 2013/07/15 22:11:44, nweiz wrote:
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > is Iterable
> > 
> > That seems a little dangerous. A lot of things can extend Iterable that may
> not
> > want to be flattened.
> 
> Conversely, a lot of things extend iterable that do want to be flattened. I
> would expect [foo.where(...)] to flatten the results of that .where() call but
> this won't do that.

That case is more straightforward to detect. If the user uses [where] or
whatever it'll be immediately clear that the code isn't working as intended,
whereas an otherwise innocuous class could suddenly start implementing Iterable
and change the behavior of the code.

For reference, Ruby's List#flatten method only works with Arrays, not
Enumerables.

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
File pkg/barback/test/utils.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/test/u...
pkg/barback/test/utils.dart:264: Map<String, Iterable<Iterable<Transformer>>>
transformers) {
On 2013/07/16 17:36:01, Bob Nystrom wrote:
> On 2013/07/15 22:11:44, nweiz wrote:
> > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > How about also allowing this to just be a nested list of transformers, and
> > > assuming a single "app" package in that case?
> > 
> > I thought about that, but it seemed weird to assume a default package name
> when
> > nothing else in the framework did. It felt weird to write "app|foo.txt" for
> all
> > the ids but then not write "app" in the transformers.
> > 
> > If you think removing some test boilerplate is worthwhile, I could make
"app"
> > the default in both cases.
> 
> Yeah, I was actually thinking about doing the former too but didn't get around
> to it. I'd be fine either way.

I'll leave it for now, but let's keep the idea in our back pocket.

https://codereview.chromium.org/5695057915019264/diff/14001/pkg/barback/lib/s...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/5695057915019264/diff/14001/pkg/barback/lib/s...
pkg/barback/lib/src/transform_node.dart:94: // have failed?
On 2013/07/16 17:36:01, Bob Nystrom wrote:
> Probably not, I think.

Right now we have no means of reporting a bad condition without considering it
an error. I'll update the TODO for that.

https://codereview.chromium.org/5695057915019264/diff/14001/pkg/barback/test/...
File pkg/barback/test/utils.dart (right):

https://codereview.chromium.org/5695057915019264/diff/14001/pkg/barback/test/...
pkg/barback/test/utils.dart:345: /// A struct used by [MockProvider] to keep
track of which assets and
On 2013/07/16 17:36:01, Bob Nystrom wrote:
> "struct" is out of place here. How about just "Used by [MockProvider] to ..."

Done.

https://codereview.chromium.org/5695057915019264/diff/14001/pkg/barback/test/...
pkg/barback/test/utils.dart:534: /// A transformer that outputs an asset with
the wrong package name.
On 2013/07/16 17:36:01, Bob Nystrom wrote:
> That may be how it's used, but it isn't what it does. What it does is take in
> any input as a primary and always outputs a fixed output with a given ID. Name
> and document accordingly.

Done.

[Bob Nystrom, 2013-07-16 22:23:17]

LGTM!

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
File pkg/barback/lib/src/asset_graph_manager.dart (right):

https://codereview.chromium.org/5695057915019264/diff/3001/pkg/barback/lib/sr...
pkg/barback/lib/src/asset_graph_manager.dart:18: // TODO(nweiz): come up with
better names for this and AssetGraph.
On 2013/07/16 19:39:39, nweiz wrote:
>  2013/07/16 17:36:01, Bob Nystrom wrote:
> > On 2013/07/15 22:11:44, nweiz wrote:
> > > On 2013/07/11 23:03:40, Bob Nystrom wrote:
> > > > Agreed. "Manager" is one of my biggest pet peeves.
> > > > 
> > > > How about "PackageGraph"?
> > > 
> > > I like "PackageGraph", but where does that leave AssetGraph? I don't want
to
> > > just call it "Package", but it won't really encapsulate an asset graph
once
> > > transforms start reaching outside a given AssetGraph for cross-package
> assets.
> > 
> > How about:
> > 
> > AssetGraphManager -> AssetGraph
> > AssetGraph -> PackageAssetGraph
> > 
> > ?
> 
> I was thinking
> 
> AssetGraphManager -> PackageGraph
> AssetGraph        -> AssetCascade
> 
> With "cascade" representing the way assets cascade through a series of phases.
> This has the side benefit of having a different noun for the two classes, so a
> variable named "_graph" consistently refers to a single type.

SGTM.

[nweiz, 2013-07-16 22:57:01]

Committed patchset #7 manually as r25079 (presubmit successful).