Add a scanner and parser for platform selectors.

Add a scanner and parser for platform selectors.

This will eventually be used by users to declare which platforms a given test or
suite supports.

See #6

R=rnystrom@google.com

Committed: https://github.com/dart-lang/unittest/commit/86eae71322ebfd80c438fd2d8344c04c59d9f5f7

[nweiz, 2015-03-10 22:15:12]

nweiz@google.com changed reviewers:
+ rnystrom@google.com

[Bob Nystrom, 2015-03-11 20:05:53]

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
File lib/src/backend/platform_selector/ast.dart (right):

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/ast.dart:52: final Node branch1;
How about "left" and "right" for these?

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/ast.dart:103: final Node branch1;
"thenBranch" and "elseBranch"?

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
File lib/src/backend/platform_selector/parser.dart (right):

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/parser.dart:18: /// consistency with
command-line arguments.
Does this add enough value to be worth the possible confusion?

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/parser.dart:49: _scanner.next();
You could simplify this (and a bunch below) by adding a match() method to
Scanner that combines the peek and next:

if (!_scanner.match(TokenType.questionMark) return condition;

...

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
File lib/src/backend/platform_selector/scanner.dart (right):

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/scanner.dart:67: throw new StateError("No more
tokens.");
Nit: you could make this a one-line if if you want.

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
File lib/src/backend/platform_selector/token.dart (right):

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/token.dart:17: /// continuous string, but the
string itself isn't actually a file. It might
This is confusing. Does that mean this should *not* be a FileSpan?

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/token.dart:24: /// An token representing an
identifier.
"An" -> "A".

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/token.dart:38: class TokenType {
Sigh, yet another case where enum should be used but can't because the feature
was too limited. :(

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
File test/backend/platform_selector/ast_test.dart (right):

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
test/backend/platform_selector/ast_test.dart:65: test("doesn't parenthesize
ors", () =>
"ands"

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
File test/backend/platform_selector/parser_test.dart (right):

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
test/backend/platform_selector/parser_test.dart:26: expect(node,
_isConditionalNode);
It's probably simpler just to test node.toString(). Since you already have AST
tests that validate that does the right thing with parentheses, that's probably
enough to validate the parse.

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
File test/backend/platform_selector/scanner_test.dart (right):

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
test/backend/platform_selector/scanner_test.dart:47: test("returns an
end-of-file token at the end of a file", () {
This is a dupe of line 18.

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
test/backend/platform_selector/scanner_test.dart:58: test("throws a StateError
if peek() is called after end-of-file was "
Ditto.

[nweiz, 2015-03-12 19:48:50]

Code review changes

[nweiz, 2015-03-12 19:48:58]

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
File lib/src/backend/platform_selector/ast.dart (right):

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/ast.dart:52: final Node branch1;
On 2015/03/11 20:05:52, Bob Nystrom wrote:
> How about "left" and "right" for these?

Done.

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/ast.dart:103: final Node branch1;
On 2015/03/11 20:05:52, Bob Nystrom wrote:
> "thenBranch" and "elseBranch"?

Decided to go with "whenTrue" and "whenFalse" instead.

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
File lib/src/backend/platform_selector/parser.dart (right):

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/parser.dart:18: /// consistency with
command-line arguments.
On 2015/03/11 20:05:52, Bob Nystrom wrote:
> Does this add enough value to be worth the possible confusion?

I think so. A lot of the identifiers that people will be using here will match
the identifiers passed on the command line, and having to remember to switch
from hyphens on the command line to underscores here (or, if you split it
differently, switch from hyphens for command-line flags to underscores for their
values) will be really painful. Dart identifiers, on the other hand, aren't
continuous with this syntax; there aren't Dart values that need to be translated
between the two, so there's less friction.

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/parser.dart:49: _scanner.next();
On 2015/03/11 20:05:52, Bob Nystrom wrote:
> You could simplify this (and a bunch below) by adding a match() method to
> Scanner that combines the peek and next:
> 
> if (!_scanner.match(TokenType.questionMark) return condition;
> 
> ...

Done.

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
File lib/src/backend/platform_selector/scanner.dart (right):

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/scanner.dart:67: throw new StateError("No more
tokens.");
On 2015/03/11 20:05:52, Bob Nystrom wrote:
> Nit: you could make this a one-line if if you want.

Done.

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
File lib/src/backend/platform_selector/token.dart (right):

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/token.dart:17: /// continuous string, but the
string itself isn't actually a file. It might
On 2015/03/11 20:05:52, Bob Nystrom wrote:
> This is confusing. Does that mean this should *not* be a FileSpan?

No; "FileSpan" is somewhat of a misnomer. It just means a span that's parsed
from a single contiguous block of text.

https://codereview.chromium.org/997593003/diff/40001/lib/src/backend/platform...
lib/src/backend/platform_selector/token.dart:24: /// An token representing an
identifier.
On 2015/03/11 20:05:53, Bob Nystrom wrote:
> "An" -> "A".

Done.

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
File test/backend/platform_selector/ast_test.dart (right):

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
test/backend/platform_selector/ast_test.dart:65: test("doesn't parenthesize
ors", () =>
On 2015/03/11 20:05:53, Bob Nystrom wrote:
> "ands"

Done.

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
File test/backend/platform_selector/parser_test.dart (right):

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
test/backend/platform_selector/parser_test.dart:26: expect(node,
_isConditionalNode);
On 2015/03/11 20:05:53, Bob Nystrom wrote:
> It's probably simpler just to test node.toString(). Since you already have AST
> tests that validate that does the right thing with parentheses, that's
probably
> enough to validate the parse.

But the AST tests also rely on the parser to do the right thing ;). I think I
prefer being explicit here, even if it may be more verbose than necessary. It
also means that if it fails, the exact point of failure is easier to track.

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
File test/backend/platform_selector/scanner_test.dart (right):

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
test/backend/platform_selector/scanner_test.dart:47: test("returns an
end-of-file token at the end of a file", () {
On 2015/03/11 20:05:53, Bob Nystrom wrote:
> This is a dupe of line 18.

It calls [scanner.next] instead of [scanner.peek].

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
test/backend/platform_selector/scanner_test.dart:58: test("throws a StateError
if peek() is called after end-of-file was "
On 2015/03/11 20:05:53, Bob Nystrom wrote:
> Ditto.

Ditto.

[Bob Nystrom, 2015-03-12 20:37:21]

LGTM!

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
File test/backend/platform_selector/scanner_test.dart (right):

https://codereview.chromium.org/997593003/diff/40001/test/backend/platform_se...
test/backend/platform_selector/scanner_test.dart:47: test("returns an
end-of-file token at the end of a file", () {
On 2015/03/12 19:48:58, nweiz wrote:
> On 2015/03/11 20:05:53, Bob Nystrom wrote:
> > This is a dupe of line 18.
> 
> It calls [scanner.next] instead of [scanner.peek].

My reading comprehension grade: F.

https://codereview.chromium.org/997593003/diff/60001/lib/src/backend/platform...
File lib/src/backend/platform_selector/parser.dart (right):

https://codereview.chromium.org/997593003/diff/60001/lib/src/backend/platform...
lib/src/backend/platform_selector/parser.dart:47: if (_scanner.peek().type !=
TokenType.questionMark) return condition;
Use _scanner.scan(TokenType.questionMark) here?

[nweiz, 2015-03-12 23:12:48]

Code review changes

[nweiz, 2015-03-12 23:12:54]

https://codereview.chromium.org/997593003/diff/60001/lib/src/backend/platform...
File lib/src/backend/platform_selector/parser.dart (right):

https://codereview.chromium.org/997593003/diff/60001/lib/src/backend/platform...
lib/src/backend/platform_selector/parser.dart:47: if (_scanner.peek().type !=
TokenType.questionMark) return condition;
On 2015/03/12 20:37:21, Bob Nystrom wrote:
> Use _scanner.scan(TokenType.questionMark) here?

Done.

[nweiz, 2015-03-12 23:13:06]

Committed patchset #5 (id:80001) manually as
86eae71322ebfd80c438fd2d8344c04c59d9f5f7 (presubmit successful).