Split AssetGraph.results into two streams.

pkg/barback/lib/src/asset_graph.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library barback.asset_graph;
 
 import 'dart:async';
 import 'dart:collection';
 
 import 'asset.dart';
 import 'asset_id.dart';
 import 'asset_provider.dart';
 import 'asset_set.dart';
 import 'errors.dart';
 import 'change_batch.dart';
 import 'phase.dart';
 import 'transformer.dart';
 
 /// The main build dependency manager.
 ///
 /// For any given input file, it can tell which output files are affected by
 /// it, and vice versa.
 class AssetGraph {
   final AssetProvider _provider;
 
   final _phases = <Phase>[];
 
+  /// A stream that emits a [BuildResult] event each time the build is

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

remove "event"

[nweiz, 2013/07/10 20:45:09]

Done.

+  /// completed, whether or not it succeeded.
+  ///
+  /// If a programming error in Barback itself occurs, it will be emitted

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

Instead of "programming error", maybe "unexpected error".

Lowercase "Barback" here and elsewhere.

[nweiz, 2013/07/10 20:45:09]

Done.
Done.

+  /// through this stream's error channel.
   Stream<BuildResult> get results => _resultsController.stream;
   final _resultsController = new StreamController<BuildResult>.broadcast();
 
+  /// A stream that emits any errors from the asset graph or the transformers.
+  ///
+  /// This emits errors as they're detected. If an error occurs in one part of
+  /// the asset graph, unrelated parts will continue building.
+  ///
+  /// This will not emit programming errors from Barback itself. Those will be
+  /// emitted through the [results] stream's error channel.
+  Stream get errors => _errorsController.stream;

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

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.

[nweiz, 2013/07/10 20:45:09]

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.

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

SGTM.

+  final _errorsController = new StreamController.broadcast();
+
+  /// A queue that accumulates errors from a single build.

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

How about "The errors that have occurred since the last build completed."

[nweiz, 2013/07/10 20:45:09]

Done, slightly reworded.

+  ///
+  /// This will be empty if no build is occurring.
+  Queue _accumulatedErrors;
+
   /// A future that completes when the currently running build process finishes.
   ///
   /// If no build it in progress, is `null`.
   Future _processDone;
 
   ChangeBatch _sourceChanges;
 
   /// Creates a new [AssetGraph].
   ///
   /// It loads source assets using [provider] and then uses [transformerPhases]
@@ skipping 71 matching lines @@
   }
 
   /// Removes [removed] from the graph's known set of source assets.
   void removeSources(Iterable<AssetId> removed) {
     if (_sourceChanges == null) _sourceChanges = new ChangeBatch();
     _sourceChanges.remove(removed);
 
     _waitForProcess();
   }
 
-  /// Reports a process result with the given error then throws it.
   void reportError(error) {
-    _resultsController.add(new BuildResult(error));
+    _accumulatedErrors.add(error);
+    _errorsController.add(error);
   }
 
   /// Starts the build process asynchronously if there is work to be done.
   ///
   /// Returns a future that completes with the background processing is done.
   /// If there is no work to do, returns a future that completes immediately.
   /// All errors that occur during processing will be caught (and routed to the
   /// [results] stream) before they get to the returned future, so it is safe
   /// to discard it.
   Future _waitForProcess() {
     if (_processDone != null) return _processDone;
+
+    _accumulatedErrors = new Queue();
     return _processDone = _process().then((_) {
       // Report the build completion.
       // TODO(rnystrom): Put some useful data in here.
-      _resultsController.add(new BuildResult());
+      _resultsController.add(new BuildResult(_accumulatedErrors));
     }).catchError((error) {
       // If we get here, it's an unexpected error. Runtime errors like missing
       // assets should be handled earlier. Errors from transformers or other
       // external code that barback calls into should be caught at that API
       // boundary.
       //
       // On the off chance we get here, pipe the error to the results stream
       // as an error. That will let applications handle it without it appearing
       // in the same path as "normal" errors that get reported.
       _resultsController.addError(error);
     }).whenComplete(() {
       _processDone = null;
+      _accumulatedErrors = null;
     });
   }
 
   /// Starts the background processing.
   ///
   /// Returns a future that completes when all assets have been processed.
   Future _process() {
     return _processSourceChanges().then((_) {
       // Find the first phase that has work to do and do it.
       var future;
@@ skipping 46 matching lines @@
         }));
       }
 
       return Future.wait(futures).then((_) {
         _phases.first.updateInputs(updated, changes.removed);
       });
     });
   }
 }
 
-/// Used to report build results back from the asynchronous build process
-/// running in the background.
+/// An event indicating that the asset graph has finished building.
+///
+/// A build can end either in success or failure. If there were no errors during
+/// the build, it's considered to be a success; any errors render it a failure,
+/// although individual assets may still have built successfully.

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

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:09]

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.

 class BuildResult {
-  /// The error that occurred, or `null` if the result is not an error.
-  final error;
+  /// All errors that occurred during the build.
+  final List errors;
 
-  /// `true` if this result is for a successful build.
-  bool get succeeded => error == null;
+  /// `true` if the build succeeded.
+  bool get succeeded => errors.isEmpty;
 
-  BuildResult([this.error]);
+  BuildResult(Iterable errors)
+      : errors = errors.toList();
 }