Add a basic YAML processor. Much of the language is still unimplemented.

Add a basic YAML processor. Much of the language is still unimplemented.

Committed: https://code.google.com/p/dart/source/detail?r=6971

[kasperl, 2012-04-20 06:02:30]

Quick style question: Why is so much of this stuff _private?

[Bob Nystrom, 2012-04-20 20:28:55]

The style is kind of strange, but it's consistent and works well if you know the
YAML spec. I'm not crazy about _(), but the _group() and _context() stuff is
reasonable.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/composer.dart
File lib/yaml/composer.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/composer.dart...
lib/yaml/composer.dart:21: * The next id to use for the represenation graph's
anchors. The spec doesn't
"id" -> "ID"

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/composer.dart...
lib/yaml/composer.dart:28: _Composer(_Node this._root) : this._anchors =
<_Node>{}, this._idCounter = 0;
Don't bother type annotating the ctor arg here.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart
File lib/yaml/model.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:28: _Tag.mapping(String name) : this(name, MAPPING_KIND);
These are nice.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:34: operator ==(var other) {
No "var" here.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:36: _Tag otherTag = other;
Unused var.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:41: if (name.startsWith('tag:yaml.org,2002:')) {
Pull this string out into a constant?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:59: _Node(_Tag this.tag, [String this.anchor]);
Don't need types for "this." params. Here and elsewhere.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:61: bool operator ==(var other) {
Remove "var" here and elsewhere.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:63: _Node otherNode = other;
You can ditch this and be consistent with _Tag.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:92: String toString() => '$tag
[${Strings.join(content.map((e) => '$e'), ', ')}]';
This map here is weird. Seems silly that Strings.join doesn't toString() its
arguments. File a bug?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:101: _AliasNode(String anchor) : super(new
_Tag.scalar(_Tag.yaml("str")), anchor);
Why "str" here?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:155: var escapedValue = value.charCodes().map((c) {
For the common case where the string doesn't contain any special characters,
this goes through a lot of churn. Maybe add a TODO saying it could be optimized
to do a single pass check for special characters and short-circuit if there
aren't any?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:190: String toString() {
=>

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:198: String _pad(String str, int length) {
"_zeroPad"?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:200: var prefix = new List();
new List() -> []

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:201: prefix.insertRange(0, length - str.length, '0');
Clever!

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart
File lib/yaml/parser.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:58: static final _C_SEQUENCE_ENTRY = 4;
Why make these private?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:96: /** The curent (0-based) column in the source string.
*/
"current"

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:126: _Parser(String s) : _s = s, _len = s.length,
I would format like:

_Parser(...)
  : _s = s,
    _len = ...

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:168: * The truthiness operator. Returns false if [obj] is
null or false, true
If you want to be fancy, consider code-formatting literals like true and null in
doc comments:  

Returns `false` if [obj] is `null` or `false`, `true`

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:221: * Just calls [consumer] and returns its result. Used
to make it explicit that a
Long line.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:242: _group(consumer()) {
This name isn't clear to me. Maybe "attempt"?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:260: bool _nAtOnce(int n, bool matcher(int, int)) =>
_group(() {
That doesn't do what you think it does. It declares that matcher takes two
Dynamic parameters whose names both happen to be "int".

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:299: _Node _addProps(_Node node, List props) {
Using a List here is kinda gross. As tedious as it is, maybe make a little Tag,
String pair type.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:306: _MappingNode _map(List<List<_Node>> pairs) {
A Pair<A, B> class would help here. The code that is called by this would I
think be definitely clearer if its annotations were returning pairs (or better,
something semantically meaningful) instead of just lists.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:308: pairs.forEach((pair) { content[pair[0]] = pair[1]; });
=>?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:325: // Line and column should be one-based
"."

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:340: * Throws an error saying that the parse failed. Uses
`_farthestLine`,
[] instead of ``.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:350: int _detectIndentation() {
"countIndentation"? I would expect "detect" to return a bool.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:357: bool get _atStartOfLine() {
=> _column == 0

?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:400: (char >= 0x10000 && char <= 0x10FFFF);
Style nit, but I would probably do one clause per line.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:444: bool _s_flowFolded(int n) => false; // TODO(nweiz):
implement
Throw UnsupportedOperationException instead. Here and elsewhere.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:489: }
Switches without defaults feel strange to me, especially since we can't rely on
static typing and flow analysis to ensure some branch gets hit. Maybe add:

default: throw 'unreachable';

or something like that. Here and elsewhere.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:494: return _group(() => _s_l_comments() &&
_s_flowLinePrefix(n)) ||
=> _group(() ... )

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:502: List _c_ns_properties(int n, int c) {
"n" isn't a helpful name. "indent"?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:585: var match = (_isPlainSafe(c, char) && indicator !=
_C_MAPPING_VALUE &&
This is kind of hard to understand. Maybe break it into separate checks?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:640: /* Indentation parameter is unused in this path */
//

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:641: var node = _ns_flowYamlNode(-10, c);
What is -10 here?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:685: _Node _ns_flowNode(int n, int c) => _or([
I'm OK with => for _group() method bodies which just wrap a single function
literal, but I don't like it for _or (or _and) where it contains a bunch of
arguments.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:729: return _or([() => _group(() {
Wow this is hard to parse. Move () => _group... onto the next line and line up
it with }), and see if that helps.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:819: if (!_s_separate(n+1, FLOW_OUT)) return null;
Spaces around + here and elsewhere.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:884: _group(() {
Couldn't you do:

return _group(() { ... });

and have the inner function return true or false instead of setting forbidden?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml.dart
File lib/yaml/yaml.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml.dart#new...
lib/yaml/yaml.dart:48: class Error implements Exception {
"YamlException"

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml_map.dart
File lib/yaml/yaml_map.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml_map.dart...
lib/yaml/yaml_map.dart:6: * This class wraps behaves almost identically to the
normal Dart Map
"wraps behaves"?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml_map.dart...
lib/yaml/yaml_map.dart:82: int _hashCode(obj) {
This will do Bad Things with cyclic structures. Does that matter? Should at the
least document this fact. (Ditto functions below.)

This is a topic close to Josh's heart, so you could ask him about it if you want
some ideas on how this should behave.

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.dart
File tests/lib/yaml_tests.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.d...
tests/lib/yaml_tests.dart:5: #library('yamlTest');
yaml_tests

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.d...
tests/lib/yaml_tests.dart:15: bool listDeepEquals(List list1, List list2) {
Instead of copying these here, you could move them into a sublibrary in yaml and
then #import it here. Putting it in a sublibrary will keep them from bleeding
out of the public API of the yaml library itself.

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.d...
tests/lib/yaml_tests.dart:119: - Mark McGwire
Unindented multiline strings make the surrounding code hard to read. For the
markdown tests, I just massaged the test strings by stripping a fixed amount of
leading indentation from each line. You should do something similar here. A more
robust way would be to count the leading indentation of the first line and strip
that from all subsequent lines. Then just make the string literals have the """
on its own line lined up with the others like:

    """
    foo
    bar
    """

https://chromiumcodereview.appspot.com/10153004/diff/7001/utils/yaml/composer...
File utils/yaml/composer.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/7001/utils/yaml/composer...
utils/yaml/composer.dart:109: if (!const
RegExp("^(null|Null|NULL|~|)\$").hasMatch(content)) return null;
Is this case-insensitive or just allowing these forms? What about nUlL?

https://chromiumcodereview.appspot.com/10153004/diff/7001/utils/yaml/parser.dart
File utils/yaml/parser.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/7001/utils/yaml/parser.d...
utils/yaml/parser.dart:171: bool _(obj) => obj != null && obj != false;
This is kind of perverse, especially the name. For cases where the object is
known to be a boolean, it's unnecessary. For other places where it isn't a
boolean, I think the explicit "!= null" is clearer.

[nweiz, 2012-04-23 23:06:33]

I've gone through and removed all the underscore prefixes on members of private
classes.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/composer.dart
File lib/yaml/composer.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/composer.dart...
lib/yaml/composer.dart:21: * The next id to use for the represenation graph's
anchors. The spec doesn't
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> "id" -> "ID"

According to Wiktionary (http://en.wiktionary.org/wiki/identifier) lower case is
valid here.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/composer.dart...
lib/yaml/composer.dart:28: _Composer(_Node this._root) : this._anchors =
<_Node>{}, this._idCounter = 0;
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Don't bother type annotating the ctor arg here.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart
File lib/yaml/model.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:34: operator ==(var other) {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> No "var" here.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:36: _Tag otherTag = other;
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Unused var.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:41: if (name.startsWith('tag:yaml.org,2002:')) {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Pull this string out into a constant?

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:59: _Node(_Tag this.tag, [String this.anchor]);
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Don't need types for "this." params. Here and elsewhere.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:61: bool operator ==(var other) {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Remove "var" here and elsewhere.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:63: _Node otherNode = other;
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> You can ditch this and be consistent with _Tag.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:92: String toString() => '$tag
[${Strings.join(content.map((e) => '$e'), ', ')}]';
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> This map here is weird. Seems silly that Strings.join doesn't toString() its
> arguments. File a bug?

Bug 2690 filed.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:101: _AliasNode(String anchor) : super(new
_Tag.scalar(_Tag.yaml("str")), anchor);
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Why "str" here?

I needed something. The spec isn't clear on what the tag should be, but "str" is
the default in other cases.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:155: var escapedValue = value.charCodes().map((c) {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> For the common case where the string doesn't contain any special characters,
> this goes through a lot of churn. Maybe add a TODO saying it could be
optimized
> to do a single pass check for special characters and short-circuit if there
> aren't any?

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:190: String toString() {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> =>

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:198: String _pad(String str, int length) {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> "_zeroPad"?

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:200: var prefix = new List();
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> new List() -> []

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/model.dart#ne...
lib/yaml/model.dart:201: prefix.insertRange(0, length - str.length, '0');
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Clever!

I'd rather write '0' * (length - str.length), but what can you do...

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart
File lib/yaml/parser.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:58: static final _C_SEQUENCE_ENTRY = 4;
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Why make these private?

Changed as part of the great de-underscoring.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:96: /** The curent (0-based) column in the source string.
*/
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> "current"

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:126: _Parser(String s) : _s = s, _len = s.length,
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> I would format like:
> 
> _Parser(...)
>   : _s = s,
>     _len = ...

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:168: * The truthiness operator. Returns false if [obj] is
null or false, true
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> If you want to be fancy, consider code-formatting literals like true and null
in
> doc comments:  
> 
> Returns `false` if [obj] is `null` or `false`, `true`

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:221: * Just calls [consumer] and returns its result. Used
to make it explicit that a
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Long line.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:242: _group(consumer()) {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> This name isn't clear to me. Maybe "attempt"?

Changed to "transaction"; slightly longer, but much clearer.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:260: bool _nAtOnce(int n, bool matcher(int, int)) =>
_group(() {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> That doesn't do what you think it does. It declares that matcher takes two
> Dynamic parameters whose names both happen to be "int".

Fixed.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:299: _Node _addProps(_Node node, List props) {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Using a List here is kinda gross. As tedious as it is, maybe make a little
Tag,
> String pair type.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:306: _MappingNode _map(List<List<_Node>> pairs) {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> A Pair<A, B> class would help here. The code that is called by this would I
> think be definitely clearer if its annotations were returning pairs (or
better,
> something semantically meaningful) instead of just lists.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:308: pairs.forEach((pair) { content[pair[0]] = pair[1]; });
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> =>?

Doesn't work with "=", because it's a statement.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:325: // Line and column should be one-based
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> "."

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:340: * Throws an error saying that the parse failed. Uses
`_farthestLine`,
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> [] instead of ``.

Done. I didn't remember that [] worked for members as well as parameters.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:350: int _detectIndentation() {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> "countIndentation"? I would expect "detect" to return a bool.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:357: bool get _atStartOfLine() {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> => _column == 0
> 
> ?

Good idea. I wrote this before I added _column.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:400: (char >= 0x10000 && char <= 0x10FFFF);
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Style nit, but I would probably do one clause per line.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:444: bool _s_flowFolded(int n) => false; // TODO(nweiz):
implement
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Throw UnsupportedOperationException instead. Here and elsewhere.

That would break parsing of supported structures completely, since these methods
are getting called.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:489: }
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Switches without defaults feel strange to me, especially since we can't rely
on
> static typing and flow analysis to ensure some branch gets hit. Maybe add:
> 
> default: throw 'unreachable';
> 
> or something like that. Here and elsewhere.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:494: return _group(() => _s_l_comments() &&
_s_flowLinePrefix(n)) ||
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> => _group(() ... )

I can't get this formatted so that everything fits cleanly on two lines when
using => there.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:502: List _c_ns_properties(int n, int c) {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> "n" isn't a helpful name. "indent"?

Changed n -> indent and c -> ctx (not "context" to avoid conflicting with the
method).

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:585: var match = (_isPlainSafe(c, char) && indicator !=
_C_MAPPING_VALUE &&
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> This is kind of hard to understand. Maybe break it into separate checks?

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:640: /* Indentation parameter is unused in this path */
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> //

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:641: var node = _ns_flowYamlNode(-10, c);
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> What is -10 here?

A value that's never referenced. Would null be better?

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:685: _Node _ns_flowNode(int n, int c) => _or([
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> I'm OK with => for _group() method bodies which just wrap a single function
> literal, but I don't like it for _or (or _and) where it contains a bunch of
> arguments.

I disagree. I can see how _or looks weird -- a list of functions isn't typical
in Dart -- but I think that's orthogonal to => vs {return}. I don't think moving
to curly braces adds any clarity here; the reader still has to learn what _or
means, but once they figure that out the more concise notation will be easier to
understand.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:729: return _or([() => _group(() {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Wow this is hard to parse. Move () => _group... onto the next line and line up
> it with }), and see if that helps.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:819: if (!_s_separate(n+1, FLOW_OUT)) return null;
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Spaces around + here and elsewhere.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:884: _group(() {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Couldn't you do:
> 
> return _group(() { ... });
> 
> and have the inner function return true or false instead of setting forbidden?

No; that would move the cursor forward if it returned "true".

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml.dart
File lib/yaml/yaml.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml.dart#new...
lib/yaml/yaml.dart:48: class Error implements Exception {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> "YamlException"

I expect people to implement this library with a prefix (hence the generic
top-level names like "load"). "yaml.YamlException" feels awkward and redundant.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml_map.dart
File lib/yaml/yaml_map.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml_map.dart...
lib/yaml/yaml_map.dart:6: * This class wraps behaves almost identically to the
normal Dart Map
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> "wraps behaves"?

Fixed.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml_map.dart...
lib/yaml/yaml_map.dart:82: int _hashCode(obj) {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> This will do Bad Things with cyclic structures. Does that matter? Should at
the
> least document this fact. (Ditto functions below.)
> 
> This is a topic close to Josh's heart, so you could ask him about it if you
want
> some ideas on how this should behave.

I've fixed hashCode and deepEquals to work properly with cyclic structures. It's
not particularly efficient, since Dart doesn't have any notion of per-object
ids, but I doubt anyone will be using map keys that are deeply-nested enough
that it'll matter.

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.dart
File tests/lib/yaml_tests.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.d...
tests/lib/yaml_tests.dart:5: #library('yamlTest');
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> yaml_tests

This is contrary to the style of the rest of tests/utils.

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.d...
tests/lib/yaml_tests.dart:15: bool listDeepEquals(List list1, List list2) {
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Instead of copying these here, you could move them into a sublibrary in yaml
and
> then #import it here. Putting it in a sublibrary will keep them from bleeding
> out of the public API of the yaml library itself.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.d...
tests/lib/yaml_tests.dart:119: - Mark McGwire
On 2012/04/20 20:28:55, Bob Nystrom wrote:
> Unindented multiline strings make the surrounding code hard to read. For the
> markdown tests, I just massaged the test strings by stripping a fixed amount
of
> leading indentation from each line. You should do something similar here. A
more
> robust way would be to count the leading indentation of the first line and
strip
> that from all subsequent lines. Then just make the string literals have the
"""
> on its own line lined up with the others like:
> 
>     """
>     foo
>     bar
>     """
> 

I feel like this is one of those cases where we should work with what the
language gives us, rather than trying to work around it. It seems kind of hacky
to filter all our multiline strings. It might read nicer if we had natively
indentation-normalized multiline strings, but we should try working with what we
do have.

Non-normalized strings also work better with tests that care about lines
containing only whitespace.

[Bob Nystrom, 2012-04-24 00:50:35]

Couple of nits and suggestions. Take what you will. Otherwise LGTM.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/composer.dart
File lib/yaml/composer.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/composer.dart...
lib/yaml/composer.dart:21: * The next id to use for the represenation graph's
anchors. The spec doesn't
On 2012/04/23 23:06:33, nweiz wrote:
> On 2012/04/20 20:28:55, Bob Nystrom wrote:
> > "id" -> "ID"
> 
> According to Wiktionary (http://en.wiktionary.org/wiki/identifier) lower case
is
> valid here.

It also has http://en.wiktionary.org/wiki/ID, but do what you like. :)

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart
File lib/yaml/parser.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:308: pairs.forEach((pair) { content[pair[0]] = pair[1]; });
On 2012/04/23 23:06:33, nweiz wrote:
> On 2012/04/20 20:28:55, Bob Nystrom wrote:
> > =>?
> 
> Doesn't work with "=", because it's a statement.

? Assignment is an expression.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:444: bool _s_flowFolded(int n) => false; // TODO(nweiz):
implement
On 2012/04/23 23:06:33, nweiz wrote:
> On 2012/04/20 20:28:55, Bob Nystrom wrote:
> > Throw UnsupportedOperationException instead. Here and elsewhere.
> 
> That would break parsing of supported structures completely, since these
methods
> are getting called.

Oh, it's relying on transaction to backtrack here? Never mind then.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:641: var node = _ns_flowYamlNode(-10, c);
On 2012/04/23 23:06:33, nweiz wrote:
> On 2012/04/20 20:28:55, Bob Nystrom wrote:
> > What is -10 here?
> 
> A value that's never referenced. Would null be better?

Yeah, plus a comment explaining that.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml.dart
File lib/yaml/yaml.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml.dart#new...
lib/yaml/yaml.dart:48: class Error implements Exception {
On 2012/04/23 23:06:33, nweiz wrote:
> On 2012/04/20 20:28:55, Bob Nystrom wrote:
> > "YamlException"
> 
> I expect people to implement this library with a prefix (hence the generic
> top-level names like "load"). "yaml.YamlException" feels awkward and
redundant.

Hmm, we should discuss this offline. So far, we very rarely use prefixes and
don't see to design our libraries assuming users will use them.

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.dart
File tests/lib/yaml_tests.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.d...
tests/lib/yaml_tests.dart:5: #library('yamlTest');
On 2012/04/23 23:06:33, nweiz wrote:
> On 2012/04/20 20:28:55, Bob Nystrom wrote:
> > yaml_tests
> 
> This is contrary to the style of the rest of tests/utils.

There's no consistent style in there. dartdoc and markdown use underscores. URI
and JSON mysteriously use camelCase, and UTF uses PascalCase. Over in
utils/tests (why do we have both of these directories? who knows),
import_mapper, peg, pub, and string_encoding use underscores.

I believe soon the style guide will recommend underscores.

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.d...
tests/lib/yaml_tests.dart:119: - Mark McGwire
On 2012/04/23 23:06:33, nweiz wrote:
> On 2012/04/20 20:28:55, Bob Nystrom wrote:
> > Unindented multiline strings make the surrounding code hard to read. For the
> > markdown tests, I just massaged the test strings by stripping a fixed amount
> of
> > leading indentation from each line. You should do something similar here. A
> more
> > robust way would be to count the leading indentation of the first line and
> strip
> > that from all subsequent lines. Then just make the string literals have the
> """
> > on its own line lined up with the others like:
> > 
> >     """
> >     foo
> >     bar
> >     """
> > 
> 
> I feel like this is one of those cases where we should work with what the
> language gives us, rather than trying to work around it. It seems kind of
hacky
> to filter all our multiline strings.

It's maybe not ideal, but it's not particularly hairy code to write, and it
makes the tests much easier to read.

> Non-normalized strings also work better with tests that care about lines
> containing only whitespace.

Right. The markdown tests have that concern too, which is why it's careful about
what leading whitespace it strips.

[Bob Nystrom, 2012-04-24 00:53:38]

https://chromiumcodereview.appspot.com/10153004/diff/14001/utils/yaml/parser....
File utils/yaml/parser.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/14001/utils/yaml/parser....
utils/yaml/parser.dart:173: bool _(obj) => obj != null && obj != false;
I sent you an LGTM, but I forgot about this one. I understand how annoying
Dart's, uh, limited concept of truthiness is, but I don't think this is the
right solution.

[nweiz, 2012-04-25 00:26:29]

PTAL at my changes to the test infrastructure.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart
File lib/yaml/parser.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:308: pairs.forEach((pair) { content[pair[0]] = pair[1]; });
On 2012/04/24 00:50:36, Bob Nystrom wrote:
> On 2012/04/23 23:06:33, nweiz wrote:
> > On 2012/04/20 20:28:55, Bob Nystrom wrote:
> > > =>?
> > 
> > Doesn't work with "=", because it's a statement.
> 
> ? Assignment is an expression.

Huh, I could have sworn this didn't work last time I checked. It works now,
though, so done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/parser.dart#n...
lib/yaml/parser.dart:641: var node = _ns_flowYamlNode(-10, c);
On 2012/04/24 00:50:36, Bob Nystrom wrote:
> On 2012/04/23 23:06:33, nweiz wrote:
> > On 2012/04/20 20:28:55, Bob Nystrom wrote:
> > > What is -10 here?
> > 
> > A value that's never referenced. Would null be better?
> 
> Yeah, plus a comment explaining that.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml.dart
File lib/yaml/yaml.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/lib/yaml/yaml.dart#new...
lib/yaml/yaml.dart:48: class Error implements Exception {
On 2012/04/24 00:50:36, Bob Nystrom wrote:
> On 2012/04/23 23:06:33, nweiz wrote:
> > On 2012/04/20 20:28:55, Bob Nystrom wrote:
> > > "YamlException"
> > 
> > I expect people to implement this library with a prefix (hence the generic
> > top-level names like "load"). "yaml.YamlException" feels awkward and
> redundant.
> 
> Hmm, we should discuss this offline. So far, we very rarely use prefixes and
> don't see to design our libraries assuming users will use them.

Done, although I'd rather see a more principled namespace system.

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.dart
File tests/lib/yaml_tests.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.d...
tests/lib/yaml_tests.dart:5: #library('yamlTest');
On 2012/04/24 00:50:36, Bob Nystrom wrote:
> On 2012/04/23 23:06:33, nweiz wrote:
> > On 2012/04/20 20:28:55, Bob Nystrom wrote:
> > > yaml_tests
> > 
> > This is contrary to the style of the rest of tests/utils.
> 
> There's no consistent style in there. dartdoc and markdown use underscores.
URI
> and JSON mysteriously use camelCase, and UTF uses PascalCase. Over in
> utils/tests (why do we have both of these directories? who knows),
> import_mapper, peg, pub, and string_encoding use underscores.
> 
> I believe soon the style guide will recommend underscores.

Changed to yaml_tests.

https://chromiumcodereview.appspot.com/10153004/diff/1/tests/lib/yaml_tests.d...
tests/lib/yaml_tests.dart:119: - Mark McGwire
On 2012/04/24 00:50:36, Bob Nystrom wrote:
> On 2012/04/23 23:06:33, nweiz wrote:
> > On 2012/04/20 20:28:55, Bob Nystrom wrote:
> > > Unindented multiline strings make the surrounding code hard to read. For
the
> > > markdown tests, I just massaged the test strings by stripping a fixed
amount
> > of
> > > leading indentation from each line. You should do something similar here.
A
> > more
> > > robust way would be to count the leading indentation of the first line and
> > strip
> > > that from all subsequent lines. Then just make the string literals have
the
> > """
> > > on its own line lined up with the others like:
> > > 
> > >     """
> > >     foo
> > >     bar
> > >     """
> > > 
> > 
> > I feel like this is one of those cases where we should work with what the
> > language gives us, rather than trying to work around it. It seems kind of
> hacky
> > to filter all our multiline strings.
> 
> It's maybe not ideal, but it's not particularly hairy code to write, and it
> makes the tests much easier to read.
> 
> > Non-normalized strings also work better with tests that care about lines
> > containing only whitespace.
> 
> Right. The markdown tests have that concern too, which is why it's careful
about
> what leading whitespace it strips.

Done.

https://chromiumcodereview.appspot.com/10153004/diff/14001/utils/yaml/parser....
File utils/yaml/parser.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/14001/utils/yaml/parser....
utils/yaml/parser.dart:173: bool _(obj) => obj != null && obj != false;
On 2012/04/24 00:53:38, Bob Nystrom wrote:
> I sent you an LGTM, but I forgot about this one. I understand how annoying
> Dart's, uh, limited concept of truthiness is, but I don't think this is the
> right solution.

Bob and I talked about this offline, and compromised on renaming this to "truth"
but continuing to use it.

[Bob Nystrom, 2012-04-25 18:46:08]

LGTM after you add a TODO like we discussed.

https://chromiumcodereview.appspot.com/10153004/diff/19001/tools/test.dart
File tools/test.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/19001/tools/test.dart#ne...
tools/test.dart:59: 'utils/tests/pub',
?

[nweiz, 2012-04-25 18:47:16]

https://chromiumcodereview.appspot.com/10153004/diff/19001/tools/test.dart
File tools/test.dart (right):

https://chromiumcodereview.appspot.com/10153004/diff/19001/tools/test.dart#ne...
tools/test.dart:59: 'utils/tests/pub',
On 2012/04/25 18:46:08, Bob Nystrom wrote:
> ?

Relic from when the tests lived in tests/lib. Removed.

[kasperl, 2012-05-24 17:48:04]

https://chromiumcodereview.appspot.com/10153004/diff/23001/tests/utils/utils....
File tests/utils/utils.status (right):

https://chromiumcodereview.appspot.com/10153004/diff/23001/tests/utils/utils....
tests/utils/utils.status:30: YamlTest: Skip # Issue 2717.
It is a really bad idea to skip failing tests. Is there a subtle reason why this
isn't marked with 'Fail'?