Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11941003:
Add an ErrorGroup class to Pub. (Closed)
|
Created:
7 years, 11 months ago by nweiz Modified:
7 years, 11 months ago Reviewers:
Bob Nystrom CC:
reviews_dartlang.org, floitsch, Lasse Reichstein Nielsen, Anders Johnsen Visibility:
Public. |
DescriptionAdd an ErrorGroup class to Pub.
Committed: https://code.google.com/p/dart/source/detail?r=17106
Patch Set 1 #
Total comments: 18
Messages
Total messages: 8 (0 generated)
LGTM. You should share this with Florian and co. too. https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart File utils/pub/error_group.dart (right): https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:38: var _isComplete = false; var -> bool https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:54: Future get complete => _complete; "completed"? The name "complete" seems odd to me here. https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:62: /// [future] that should be used in its place. Document that it's an error to register an already-completed future? https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:66: Future registerFuture(Future future) { What do you think of just "addFuture" instead of "register"? https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:86: /// error, it's a [StateError] to try to register a new [Stream]. Document that it's an error to register an already-closed stream? https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:138: _streams.every((stream) => stream._isComplete); Indent another 2. https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:147: _streams.every((stream) => stream._isComplete); Ditto. https://codereview.chromium.org/11941003/diff/1/utils/tests/pub/error_group_t... File utils/tests/pub/error_group_test.dart (right): https://codereview.chromium.org/11941003/diff/1/utils/tests/pub/error_group_t... utils/tests/pub/error_group_test.dart:455: // TODO(nweiz): remove this once it's built in to unittest File a bug?
+floitsch, lrn, ajohnsen This is my implementation of the error grouping that Bob and I were talking about. We're planning on using it in pub to wrap the dart:io Process API. https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart File utils/pub/error_group.dart (right): https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:38: var _isComplete = false; On 2013/01/15 22:49:47, Bob Nystrom wrote: > var -> bool You've told me in the past not to use type declarations if they're inferrable from the RHS. https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:54: Future get complete => _complete; On 2013/01/15 22:49:47, Bob Nystrom wrote: > "completed"? The name "complete" seems odd to me here. Changed to "done" as per offline discussion. https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:62: /// [future] that should be used in its place. On 2013/01/15 22:49:47, Bob Nystrom wrote: > Document that it's an error to register an already-completed future? It's not. It's a little silly, but it'll work fine. https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:66: Future registerFuture(Future future) { On 2013/01/15 22:49:47, Bob Nystrom wrote: > What do you think of just "addFuture" instead of "register"? I think that makes it sound a little too much like a collection, which could cause some inaccurate assumptions. https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:86: /// error, it's a [StateError] to try to register a new [Stream]. On 2013/01/15 22:49:47, Bob Nystrom wrote: > Document that it's an error to register an already-closed stream? See above. https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:138: _streams.every((stream) => stream._isComplete); On 2013/01/15 22:49:47, Bob Nystrom wrote: > Indent another 2. Done. https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:147: _streams.every((stream) => stream._isComplete); On 2013/01/15 22:49:47, Bob Nystrom wrote: > Ditto. Done. https://codereview.chromium.org/11941003/diff/1/utils/tests/pub/error_group_t... File utils/tests/pub/error_group_test.dart (right): https://codereview.chromium.org/11941003/diff/1/utils/tests/pub/error_group_t... utils/tests/pub/error_group_test.dart:455: // TODO(nweiz): remove this once it's built in to unittest On 2013/01/15 22:49:47, Bob Nystrom wrote: > File a bug? Done.
Message was sent while issue was closed.
https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart File utils/pub/error_group.dart (right): https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:38: var _isComplete = false; On 2013/01/16 00:12:10, nweiz wrote: > On 2013/01/15 22:49:47, Bob Nystrom wrote: > > var -> bool > > You've told me in the past not to use type declarations if they're inferrable > from the RHS. I think the rough guideline we've followed is to do that for final fields, but not mutable ones.
Message was sent while issue was closed.
https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart File utils/pub/error_group.dart (right): https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... utils/pub/error_group.dart:38: var _isComplete = false; On 2013/01/16 18:03:44, Bob Nystrom wrote: > On 2013/01/16 00:12:10, nweiz wrote: > > On 2013/01/15 22:49:47, Bob Nystrom wrote: > > > var -> bool > > > > You've told me in the past not to use type declarations if they're inferrable > > from the RHS. > > I think the rough guideline we've followed is to do that for final fields, but > not mutable ones. ...why? It's no less inferable in the "var" case than in the "final" case. We don't manually annotate "var" declarations outside of member variables. That just seems inconsistent.
Message was sent while issue was closed.
On 2013/01/16 19:29:11, nweiz wrote: > https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart > File utils/pub/error_group.dart (right): > > https://codereview.chromium.org/11941003/diff/1/utils/pub/error_group.dart#ne... > utils/pub/error_group.dart:38: var _isComplete = false; > On 2013/01/16 18:03:44, Bob Nystrom wrote: > > On 2013/01/16 00:12:10, nweiz wrote: > > > On 2013/01/15 22:49:47, Bob Nystrom wrote: > > > > var -> bool > > > > > > You've told me in the past not to use type declarations if they're > inferrable > > > from the RHS. > > > > I think the rough guideline we've followed is to do that for final fields, but > > not mutable ones. > > ...why? It's no less inferable in the "var" case than in the "final" case. We > don't manually annotate "var" declarations outside of member variables. That > just seems inconsistent. With final variables, the *only* type it can ever have is the type of the initializer. With "var", you could assign to a different type later. For example: var assignedToLater = null; Using "var" here doesn't tell us what type it will end up having.
Message was sent while issue was closed.
On 2013/01/16 21:30:50, Bob Nystrom wrote: > With final variables, the *only* type it can ever have is the type of the > initializer. With "var", you could assign to a different type later. For > example: > > var assignedToLater = null; > > Using "var" here doesn't tell us what type it will end up having. With final declarations, we include a type when it can't be inferred -- usually when it's being set in the constructor, but potentially also when it's null (e.g. for overriding an inherited property). Similarly, we should type annotate "var" if and only if it's not inferrable.
Message was sent while issue was closed.
On 2013/01/16 21:33:31, nweiz wrote: > On 2013/01/16 21:30:50, Bob Nystrom wrote: > > With final variables, the *only* type it can ever have is the type of the > > initializer. With "var", you could assign to a different type later. For > > example: > > > > var assignedToLater = null; > > > > Using "var" here doesn't tell us what type it will end up having. > > With final declarations, we include a type when it can't be inferred -- usually > when it's being set in the constructor, but potentially also when it's null > (e.g. for overriding an inherited property). Similarly, we should type annotate > "var" if and only if it's not inferrable. SGTM. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
