Add new constructors to YamlNode subclasses.

Add new constructors to YamlNode subclasses.

These constructors make it easy to use the same code for extracting data from
both YAML and non-YAML objects.

R=rnystrom@google.com

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

[Bob Nystrom, 2014-06-21 00:14:15]

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/dummy_s...
File pkg/yaml/lib/src/dummy_span.dart (right):

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/dummy_s...
pkg/yaml/lib/src/dummy_span.dart:15: class DummySpan extends Span {
How about "Dummy" -> "Null"?

It's effectively the null object pattern, and "dummy" has mocking or testing
implications to me.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/dummy_s...
pkg/yaml/lib/src/dummy_span.dart:22: DummySpan._(Location location)
What's this constructor for?

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
File pkg/yaml/lib/src/yaml_node.dart (right):

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node.dart:47: /// non-[YamlNode] values, but [Map.keys]
and [Map.forEach] will always expose
I didn't follow what "allow keys to be accessed using non-[YamlNode] values"
means. Can you clarify?

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node.dart:59: /// [Span.getLocationMessage]. If
[sourceName] is passed, it's used as
"as" -> "as the"

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node.dart:118: /// Users of the library should not use
this constructor.
For cases like this, I like the pattern of making the method (here ctor) private
and adding a top-level, non-exported function to invoke it.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
File pkg/yaml/lib/src/yaml_node_wrapper.dart (right):

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node_wrapper.dart:9: import
'package:collection/collection.dart' as pc;
Why "pc"? How about "collection"?

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node_wrapper.dart:40: if (value is List) return new
YamlListWrapper._(value, span);
Since the wrapper classes don't implement equality, returning a new one each
time can cause some surprising behavior:

var map = new YamlMap.wrap({"key": {"inner": "map"});
print(map["key"] == map["key"]); // false

We could memoize the wrapped objects, or maybe implement equality. The former
has better semantics but worse memory usage. What do you think?

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node_wrapper.dart:63: if (value is List) return new
YamlListWrapper._(value, _span);
Encapsulate this little blob of logic in a helper fn and use here and below.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node_wrapper.dart:82: throw new UnsupportedError("Cannot
modify an unmodifiable List");
"."

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node_wrapper.dart:101: throw new UnsupportedError("Cannot
modify an unmodifiable List");
"."

[nweiz, 2014-06-23 22:23:07]

I've merged in the other YAML changes and bumped the version to 1.1.0.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/dummy_s...
File pkg/yaml/lib/src/dummy_span.dart (right):

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/dummy_s...
pkg/yaml/lib/src/dummy_span.dart:15: class DummySpan extends Span {
On 2014/06/21 00:14:15, Bob Nystrom wrote:
> How about "Dummy" -> "Null"?
> 
> It's effectively the null object pattern, and "dummy" has mocking or testing
> implications to me.

Done.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/dummy_s...
pkg/yaml/lib/src/dummy_span.dart:22: DummySpan._(Location location)
On 2014/06/21 00:14:15, Bob Nystrom wrote:
> What's this constructor for?

The superclass constructor requires two locations, but we want to pass the same
object for both. This constructor exists so the previous constructor can pass it
a location that it can then use twice.

Basically, it's a band-aid for Dart's inability to use local variables in
initializer lists.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
File pkg/yaml/lib/src/yaml_node.dart (right):

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node.dart:47: /// non-[YamlNode] values, but [Map.keys]
and [Map.forEach] will always expose
On 2014/06/21 00:14:15, Bob Nystrom wrote:
> I didn't follow what "allow keys to be accessed using non-[YamlNode] values"
> means. Can you clarify?

Done.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node.dart:59: /// [Span.getLocationMessage]. If
[sourceName] is passed, it's used as
On 2014/06/21 00:14:15, Bob Nystrom wrote:
> "as" -> "as the"

Done.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node.dart:118: /// Users of the library should not use
this constructor.
On 2014/06/21 00:14:15, Bob Nystrom wrote:
> For cases like this, I like the pattern of making the method (here ctor)
private
> and adding a top-level, non-exported function to invoke it.

I disagree. That adds a lot of overhead to internal uses of the class for very
little benefit.

The language's privacy rules are never going to be able to perfectly enforce a
library's usage restrictions—for example, a user could always just import
something from src. Some of those restrictions have to be encoded in the
documentation instead. Given how clear the documentation is here, I think it's
fine not to bend over backwards to make it language-private as well.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
File pkg/yaml/lib/src/yaml_node_wrapper.dart (right):

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node_wrapper.dart:9: import
'package:collection/collection.dart' as pc;
On 2014/06/21 00:14:15, Bob Nystrom wrote:
> Why "pc"? How about "collection"?

"pc" is for "package collection". Just calling it "collection" is confusing
because the point of prefixing it in the first place is to disambiguate between
two collection libraries.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node_wrapper.dart:40: if (value is List) return new
YamlListWrapper._(value, span);
On 2014/06/21 00:14:15, Bob Nystrom wrote:
> Since the wrapper classes don't implement equality, returning a new one each
> time can cause some surprising behavior:
> 
> var map = new YamlMap.wrap({"key": {"inner": "map"});
> print(map["key"] == map["key"]); // false
> 
> We could memoize the wrapped objects, or maybe implement equality. The former
> has better semantics but worse memory usage. What do you think?

I like implementing equality in terms of the wrapped objects' equality. Done.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node_wrapper.dart:63: if (value is List) return new
YamlListWrapper._(value, _span);
On 2014/06/21 00:14:15, Bob Nystrom wrote:
> Encapsulate this little blob of logic in a helper fn and use here and below.

Done.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node_wrapper.dart:82: throw new UnsupportedError("Cannot
modify an unmodifiable List");
On 2014/06/21 00:14:15, Bob Nystrom wrote:
> "."

Done, although this now doesn't match the "dart:" unmodifiable views' error
messages.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node_wrapper.dart:101: throw new UnsupportedError("Cannot
modify an unmodifiable List");
On 2014/06/21 00:14:15, Bob Nystrom wrote:
> "."

Done.

[Bob Nystrom, 2014-06-23 23:11:44]

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/dummy_s...
File pkg/yaml/lib/src/dummy_span.dart (right):

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/dummy_s...
pkg/yaml/lib/src/dummy_span.dart:22: DummySpan._(Location location)
On 2014/06/23 22:23:06, nweiz wrote:
> On 2014/06/21 00:14:15, Bob Nystrom wrote:
> > What's this constructor for?
> 
> The superclass constructor requires two locations, but we want to pass the
same
> object for both. This constructor exists so the previous constructor can pass
it
> a location that it can then use twice.
> 
> Basically, it's a band-aid for Dart's inability to use local variables in
> initializer lists.

Oh, I see it now. Thanks.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
File pkg/yaml/lib/src/yaml_node.dart (right):

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node.dart:118: /// Users of the library should not use
this constructor.
On 2014/06/23 22:23:06, nweiz wrote:
> On 2014/06/21 00:14:15, Bob Nystrom wrote:
> > For cases like this, I like the pattern of making the method (here ctor)
> private
> > and adding a top-level, non-exported function to invoke it.
> 
> I disagree. That adds a lot of overhead to internal uses of the class for very
> little benefit.
> 
> The language's privacy rules are never going to be able to perfectly enforce a
> library's usage restrictions—for example, a user could always just import
> something from src.

I don't look at it as preventing the user from doing something so much as
avoiding clutter in the API.

> Some of those restrictions have to be encoded in the
> documentation instead. 

Sure, but not this. It's not like the pattern is that hard. Barback uses it in a
number of places. What's different here?

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
File pkg/yaml/lib/src/yaml_node_wrapper.dart (right):

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node_wrapper.dart:9: import
'package:collection/collection.dart' as pc;
On 2014/06/23 22:23:07, nweiz wrote:
> On 2014/06/21 00:14:15, Bob Nystrom wrote:
> > Why "pc"? How about "collection"?
> 
> "pc" is for "package collection". Just calling it "collection" is confusing
> because the point of prefixing it in the first place is to disambiguate
between
> two collection libraries.

I didn't infer that abbreviation at all. The other import isn't prefixed, so
it's not like "collection" is in use. If you don't want that, at least,
"pkg_collection" or something along those lines. "pc" didn't mean anything to
me.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node_wrapper.dart:82: throw new UnsupportedError("Cannot
modify an unmodifiable List");
On 2014/06/23 22:23:07, nweiz wrote:
> On 2014/06/21 00:14:15, Bob Nystrom wrote:
> > "."
> 
> Done, although this now doesn't match the "dart:" unmodifiable views' error
> messages.

Ugh. Well, these are better. :)

[nweiz, 2014-06-24 00:03:09]

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
File pkg/yaml/lib/src/yaml_node.dart (right):

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node.dart:118: /// Users of the library should not use
this constructor.
On 2014/06/23 23:11:44, Bob Nystrom wrote:
> On 2014/06/23 22:23:06, nweiz wrote:
> > On 2014/06/21 00:14:15, Bob Nystrom wrote:
> > > For cases like this, I like the pattern of making the method (here ctor)
> > private
> > > and adding a top-level, non-exported function to invoke it.
> > 
> > I disagree. That adds a lot of overhead to internal uses of the class for
very
> > little benefit.
> > 
> > The language's privacy rules are never going to be able to perfectly enforce
a
> > library's usage restrictions—for example, a user could always just import
> > something from src.
> 
> I don't look at it as preventing the user from doing something so much as
> avoiding clutter in the API.

It would be nice to be able to tell docgen "don't document this member", but I'd
rather not make the code more complicated to work around the lack of that
feature.

> > Some of those restrictions have to be encoded in the
> > documentation instead. 
> 
> Sure, but not this. It's not like the pattern is that hard.

It's pretty gross. It involves an unnecessary level of indirection that's
confusing for anyone reading the code that uses it.

> Barback uses it in a number of places. What's different here?

I'm not a big fan of the pattern in Barback either. I think you introduced it.

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
File pkg/yaml/lib/src/yaml_node_wrapper.dart (right):

https://codereview.chromium.org/343063002/diff/40001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node_wrapper.dart:9: import
'package:collection/collection.dart' as pc;
On 2014/06/23 23:11:44, Bob Nystrom wrote:
> On 2014/06/23 22:23:07, nweiz wrote:
> > On 2014/06/21 00:14:15, Bob Nystrom wrote:
> > > Why "pc"? How about "collection"?
> > 
> > "pc" is for "package collection". Just calling it "collection" is confusing
> > because the point of prefixing it in the first place is to disambiguate
> between
> > two collection libraries.
> 
> I didn't infer that abbreviation at all. The other import isn't prefixed, so
> it's not like "collection" is in use.

The point of using a prefix is to make it clear where the name is coming from,
and "collection" doesn't do that when there are two collections libraries.

> If you don't want that, at least,
> "pkg_collection" or something along those lines. "pc" didn't mean anything to
> me.

That's so verbose, but done.

[Bob Nystrom, 2014-06-24 00:38:22]

lgtm

[nweiz, 2014-06-24 18:38:34]

Committed patchset #6 manually as r37665 (presubmit successful).