Issue 10377186: Support a much larger subset of YAML.

Issue 10377186: Support a much larger subset of YAML. (Closed)

Created:
8 years, 7 months ago by nweiz

Modified:
8 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

Support a much larger subset of YAML. With this change, the only parts of the syntax we don't parse are directives, tags, and anchors. Committed: https://code.google.com/p/dart/source/detail?r=7771

Patch Set 1 #

Patch Set 2 : New chunks I guess #

Total comments: 16

Patch Set 3 : Code review chagnes #

Created: 8 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1888 lines, -930 lines)			Patch
M	utils/pub/yaml/composer.dart	View	1 2	1 chunk	+6 lines, -2 lines	0 comments	Download
M	utils/pub/yaml/parser.dart	View	1 2	37 chunks	+977 lines, -64 lines	0 comments	Download
M	utils/pub/yaml/yaml_map.dart	View		2 chunks	+4 lines, -0 lines	0 comments	Download
M	utils/tests/pub/yaml_test.dart	View		19 chunks	+901 lines, -864 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Bob Nystrom

I didn't try to actually grok every line of code in the parser, but LGTM. ...

8 years, 7 months ago (2012-05-18 21:06:30 UTC) #2

nweiz

8 years, 7 months ago (2012-05-18 21:47:03 UTC) #3

https://chromiumcodereview.appspot.com/10377186/diff/5002/utils/pub/yaml/comp...
File utils/pub/yaml/composer.dart (right):

https://chromiumcodereview.appspot.com/10377186/diff/5002/utils/pub/yaml/comp...
utils/pub/yaml/composer.dart:164: var infStr = match.group(1) == "-" ?
"-Infinity" : "Infinity";
On 2012/05/18 21:06:30, Bob Nystrom wrote:
> wflly shrt nm. How about "infinity"?

Done.

https://chromiumcodereview.appspot.com/10377186/diff/5002/utils/pub/yaml/pars...
File utils/pub/yaml/parser.dart (right):

https://chromiumcodereview.appspot.com/10377186/diff/5002/utils/pub/yaml/pars...
utils/pub/yaml/parser.dart:156: * The buffer containing string currently being
captured.
On 2012/05/18 21:06:30, Bob Nystrom wrote:
> "string" -> "the string"

Done.

https://chromiumcodereview.appspot.com/10377186/diff/5002/utils/pub/yaml/pars...
utils/pub/yaml/parser.dart:295: oldCaptureStart = captureStart;
On 2012/05/18 21:06:30, Bob Nystrom wrote:
> How about splitting these into separate declarations? Using "," seems a bit
> gratuitous.

Done.

https://chromiumcodereview.appspot.com/10377186/diff/5002/utils/pub/yaml/pars...
utils/pub/yaml/parser.dart:345: if (capturedString != null) throw 'captureString
calls may not be nested';
On 2012/05/18 21:06:30, Bob Nystrom wrote:
> This is a programmatic error, right? If so, I would use assert().

Done.

https://chromiumcodereview.appspot.com/10377186/diff/5002/utils/pub/yaml/pars...
utils/pub/yaml/parser.dart:351: captureStart = capturedString = null;
On 2012/05/18 21:06:30, Bob Nystrom wrote:
> This chained assignment doesn't really add a lot of value and might cause a
> static type warning. Split into separate assignments?

Done.

https://chromiumcodereview.appspot.com/10377186/diff/5002/utils/pub/yaml/pars...
utils/pub/yaml/parser.dart:357: captureStart = capturedString = null;
On 2012/05/18 21:06:30, Bob Nystrom wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10377186/diff/5002/utils/pub/yaml/pars...
utils/pub/yaml/parser.dart:362: captureAndTransform(consumer, (_) =>
replacement);
On 2012/05/18 21:06:30, Bob Nystrom wrote:
> +2 indent.

Done.

https://chromiumcodereview.appspot.com/10377186/diff/5002/utils/pub/yaml/pars...
utils/pub/yaml/parser.dart:471: "leading empty lines may not be indented more
than the first "
On 2012/05/18 21:06:30, Bob Nystrom wrote:
> Sentence case and ".".

Done.

Expand Messages | Collapse Messages