Split AssetGraph.results into two streams.

Split AssetGraph.results into two streams.

R=rnystrom@google.com

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

[Bob Nystrom, 2013-07-10 20:19:44]

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:28: /// A stream that emits a [BuildResult]
event each time the build is
remove "event"

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:31: /// If a programming error in Barback
itself occurs, it will be emitted
Instead of "programming error", maybe "unexpected error".

Lowercase "Barback" here and elsewhere.

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:43: Stream get errors =>
_errorsController.stream;
I think we want a different name for this. Maybe "updates" or "events" or
something? We will very likely also use it for notifying when individual
transforms complete successfully, and for warnings. I'm guessing we'll also add
a way for transforms to log stuff and that will likely go through this.

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:46: /// A queue that accumulates errors
from a single build.
How about "The errors that have occurred since the last build completed."

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:245: /// although individual assets may
still have built successfully.
I don't think it's meaningful to say a build can succeed or fail. Parts of it
may succeed or fail, but there isn't a single overall output, so I don't think
there's anything special about the case where all transforms happened to
succeed.

[nweiz, 2013-07-10 20:45:08]

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:28: /// A stream that emits a [BuildResult]
event each time the build is
On 2013/07/10 20:19:44, Bob Nystrom wrote:
> remove "event"

Done.

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:31: /// If a programming error in Barback
itself occurs, it will be emitted
On 2013/07/10 20:19:44, Bob Nystrom wrote:
> Instead of "programming error", maybe "unexpected error".

Done.

> Lowercase "Barback" here and elsewhere.

Done.

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:43: Stream get errors =>
_errorsController.stream;
On 2013/07/10 20:19:44, Bob Nystrom wrote:
> I think we want a different name for this. Maybe "updates" or "events" or
> something? We will very likely also use it for notifying when individual
> transforms complete successfully, and for warnings. I'm guessing we'll also
add
> a way for transforms to log stuff and that will likely go through this.

I'd rather put different types of events in different streams.

In any case, the name now should reflect the events that are emitted now. If we
refactor to change those events in the future, that's when we should change the
name, especially since the API will need to change as well.

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:46: /// A queue that accumulates errors
from a single build.
On 2013/07/10 20:19:44, Bob Nystrom wrote:
> How about "The errors that have occurred since the last build completed."

Done, slightly reworded.

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:245: /// although individual assets may
still have built successfully.
On 2013/07/10 20:19:44, Bob Nystrom wrote:
> I don't think it's meaningful to say a build can succeed or fail. Parts of it
> may succeed or fail, but there isn't a single overall output, so I don't think
> there's anything special about the case where all transforms happened to
> succeed.

I disagree. For users who care about the entire build (e.g. pub deploy) the
distinction between "everything was successful" and "something failed" is
definitely relevant. It's also easy to ignore the [succeeded] field for users
who don't care about it.

It's also telling that our tests care about this notion of success/failure.

[Bob Nystrom, 2013-07-10 21:38:16]

LGTM.

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/18178021/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:43: Stream get errors =>
_errorsController.stream;
On 2013/07/10 20:45:09, nweiz wrote:
> On 2013/07/10 20:19:44, Bob Nystrom wrote:
> > I think we want a different name for this. Maybe "updates" or "events" or
> > something? We will very likely also use it for notifying when individual
> > transforms complete successfully, and for warnings. I'm guessing we'll also
> add
> > a way for transforms to log stuff and that will likely go through this.
> 
> I'd rather put different types of events in different streams.
> 
> In any case, the name now should reflect the events that are emitted now. If
we
> refactor to change those events in the future, that's when we should change
the
> name, especially since the API will need to change as well.

SGTM.

[nweiz, 2013-07-10 22:15:39]

Committed patchset #2 manually as r24892 (presubmit successful).