Issue 183813006: Use zones to capture unexpected errors in barback.

Issue 183813006: Use zones to capture unexpected errors in barback. (Closed)

Created:
6 years, 9 months ago by nweiz

Modified:
6 years, 7 months ago

Reviewers:
Bob Nystrom

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Use zones to capture unexpected errors in barback. Rather than being extremely diligent about capturing and piping asynchronous errors within barback, we can now rely on the zone API to put all of them where they belong. R=rnystrom@google.com

Patch Set 1 #

Total comments: 8

Patch Set 2 : code review #

Created: 6 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+82 lines, -69 lines)			Patch
M	pkg/barback/lib/src/asset_cascade.dart	View		1 chunk	+0 lines, -11 lines	0 comments	Download
M	pkg/barback/lib/src/package_graph.dart	View	1	5 chunks	+82 lines, -54 lines	0 comments	Download
M	pkg/barback/lib/src/phase_input.dart	View		1 chunk	+0 lines, -4 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Bob Nystrom

Couple of suggestions then LGTM. https://codereview.chromium.org/183813006/diff/1/pkg/barback/lib/src/package_graph.dart File pkg/barback/lib/src/package_graph.dart (right): https://codereview.chromium.org/183813006/diff/1/pkg/barback/lib/src/package_graph.dart#newcode105 pkg/barback/lib/src/package_graph.dart:105: // If any cascade ...

6 years, 9 months ago (2014-03-04 23:52:40 UTC) #2

nweiz

6 years, 9 months ago (2014-03-05 00:11:57 UTC) #3

https://codereview.chromium.org/183813006/diff/1/pkg/barback/lib/src/package_...
File pkg/barback/lib/src/package_graph.dart (right):

https://codereview.chromium.org/183813006/diff/1/pkg/barback/lib/src/package_...
pkg/barback/lib/src/package_graph.dart:105: // If any cascade hasn't yet
finished, the overall build isn't finished
On 2014/03/04 23:52:40, Bob Nystrom wrote:
> Long line.

Done.

> This method is pretty big. How about breaking the callbacks based to listen()
> here and above into separate methods?

Done.

https://codereview.chromium.org/183813006/diff/1/pkg/barback/lib/src/package_...
pkg/barback/lib/src/package_graph.dart:155: return results.first.then((_) =>
getAllAssets()) ;
On 2014/03/04 23:52:40, Bob Nystrom wrote:
> Extra space.

Done.

https://codereview.chromium.org/183813006/diff/1/pkg/barback/lib/src/package_...
pkg/barback/lib/src/package_graph.dart:210: /// to the returned [Future] as
well. ANy other exceptions will be piped to
On 2014/03/04 23:52:40, Bob Nystrom wrote:
> "ANy" -> "Any"

Done.

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

https://codereview.chromium.org/183813006/diff/1/pkg/barback/lib/src/phase_in...
pkg/barback/lib/src/phase_input.dart:206: // results (issue 16162).
On 2014/03/04 23:52:40, Bob Nystrom wrote:
> Is this TODO still relevant?

Yes; these still need to be wrapped as BarbackExceptions.

Expand Messages | Collapse Messages