-
Notifications
You must be signed in to change notification settings - Fork 64
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -39,15 +39,89 @@ abstract class Expression implements Spec { | |||
return new BinaryExpression._(toExpression(), other, '&&'); | |||
} | |||
|
|||
/// This expression preceding by `await`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "preceded"
} | ||
|
||
/// Return `{other} = {this}`. | ||
Expression assign(Expression other) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, a.assign(b)
reads way too much like a = b
. Maybe flip the logic and make it {this} = {other}
?
Or perhaps rename to assignTo
? Although not sure that helps much.
); | ||
} | ||
|
||
/// Return `{other} ??= {this}`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as the one with assign
.
); | ||
} | ||
|
||
/// May be overriden to support other types implementing [Expression]. | ||
/// Returns a const instance of this expression with a named constructor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what this does. Could you provide an example (and add it to the dartdoc)? Specifically, I'm not aware of many kinds of expressions that you can invoke with a named constructor, i.e. only references to classes. Perhaps this method should be moved to Reference
?
); | ||
} | ||
|
||
/// This expression preceding by `return`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: preceded
Going to fix some of the comments in the next CL. |
* Fix a number of issues. * OK, add some tests. * Dartfmt.
Closes dart-lang/tools#857.
Closes dart-lang/tools#858.
Closes dart-lang/tools#859.
Closes dart-lang/tools#860.
Closes dart-lang/tools#861.
This API already exists internally, so I'm skipping tests until we are back in sync - I won't release this asEDIT: Added tests anyway.2.0.0-alpha+3
before adding test coverage, though.