Add a MapRule

Add a MapRule

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

[Alan Knight, 2013-01-23 23:57:29]

Adds a MapRule. This is trickier than I would have liked. Previous code tended
to assume, like JSON, that if something is a Map, then it's a map with string
keys and arbitrary values. With general maps, where we need to make references
for the keys as well this is trickier, and we need to distinguish the two. In
addition, in order to improve readability in JSON type formats, if the data is a
map that does have string keys we keep it that way instead of generating
references for the strings.

Some changes to make tests less brittle if we change the default numbering of
rules.

This changes the contract between rules and the writer. I was trying to be very
agnostic as to the format of the data that the rule stores, but it's too
difficult to know nothing. So the assumption is that the data is stored either
as lists, maps, or as primitives, and for a particular serialization and a
particular rule that doesn't change.

Because modifying the keys as a map is messy (particularly if you're iterating
over the map) and inefficient, flatten() is changed to have a return value,
which, if non-null, will replace the previous state.

Introduced an abstract ReaderWriter superclass, which doesn't do much, but at
least allows us to type things as containing one of those two, which would have
more easily detected a stupid error I'd made.

Has a really ugly hack in there because MappedList, which I'm not convinced
should even exist, has a number of problems.

[Jennifer Messerly, 2013-01-25 01:19:59]

lgtm

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/basic_rule.dart (right):

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/basic_rule.dart:155: state[inflated] = state[each];
fwiw, I think you could do:

    state[inflated] = state.remove(each);

Not sure if Map allows that kind of mutation while iterating keys.  But since it
allows adding items, I guess it probably works. Would be good to test against
both HashMap and LinkedHashMap

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/basic_rule.dart:622: var _map = new Map();
final?

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/format.dart (right):

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/format.dart:160: var rootRule = (data is Map) ?
data["__rule"] : data.last;
hmmm. Do we know for sure "data" is an Iterable or a Map? If not, would be good
to throw an ArgumentError. It helps people know what went wrong, otherwise the
error will be something like "NoSuchMethod last"

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/format.dart:179: // TODO(alanknight): Ugh. Get rid of
this if we can resolve bug 7982/7940.
or http://dartbug.com/8113 :)

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/format.dart:321: // serialization operation along with
the reader and having it as a field.
yeah, it is kind of nice how Format is independent of Reader though. I guess if
the format took the Reader at construction. But it's strange because IIRC,
Reader also takes a Format?

another idea: maybe split Format into two things, the reading and writing side?
then holding onto a reader seems less strange.

Or just don't worry about the extra argument :)

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/reader_writer.dart (right):

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/reader_writer.dart:238: abstract class ReaderWriter {
hmmm, I would expect ReaderWriter to do both reading and writing. Not sure what
other name to use though... "SerializerBase"?

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/serialization_rule.dart (right):

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/serialization_rule.dart:220: bool keysAreAllStrings =
state.keys.every((x) => x is String);
out of curiousity, why do we check all keys? Could we check each key individual
and preserve strings when possible?

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/serialization_rule.dart:222:
keysAndValues(state).forEach(
I would have expected a return in both branches. Could we "return state;" here?

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/test/ser...
File pkg/serialization/test/serialization_test.dart (right):

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/test/ser...
pkg/serialization/test/serialization_test.dart:51: (x) => x is BasicRule &&
x.type == reflect(p1).type);
Do type literals work now? if so could this be "x.type == Person"?

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/test/ser...
pkg/serialization/test/serialization_test.dart:390: var reader = s.newReader(new
SimpleJsonFormat(storeRoundTripInfo: true));
out of curiosity, would it work to use the same SimpleJsonFormat for both reader
and writer?

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/test/ser...
pkg/serialization/test/serialization_test.dart:517: reader.data = new
List(10).mappedBy((x) => [sampleData]).toList();
Would it work to use "new List.filled(10, [sampleData])" here?

[Alan Knight, 2013-01-26 01:33:38]

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/basic_rule.dart (right):

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/basic_rule.dart:155: state[inflated] = state[each];
On 2013/01/25 01:19:59, John Messerly wrote:
> fwiw, I think you could do:
> 
>     state[inflated] = state.remove(each);
> 
> Not sure if Map allows that kind of mutation while iterating keys.  But since
it
> allows adding items, I guess it probably works. Would be good to test against
> both HashMap and LinkedHashMap

I had that at one point and it appeared to work, but it made me extremely
nervous of discovering that it only worked most of the time, or suddenly would
stop working, so I decided it was better not to do it.

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/basic_rule.dart:622: var _map = new Map();
On 2013/01/25 01:19:59, John Messerly wrote:
> final?

Done.

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/format.dart (right):

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/format.dart:160: var rootRule = (data is Map) ?
data["__rule"] : data.last;
On 2013/01/25 01:19:59, John Messerly wrote:
> hmmm. Do we know for sure "data" is an Iterable or a Map? If not, would be
good
> to throw an ArgumentError. It helps people know what went wrong, otherwise the
> error will be something like "NoSuchMethod last"

Done.

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/format.dart:321: // serialization operation along with
the reader and having it as a field.
On 2013/01/25 01:19:59, John Messerly wrote:
> yeah, it is kind of nice how Format is independent of Reader though. I guess
if
> the format took the Reader at construction. But it's strange because IIRC,
> Reader also takes a Format?
> 
> another idea: maybe split Format into two things, the reading and writing
side?
> then holding onto a reader seems less strange.
> 
> Or just don't worry about the extra argument :)

For the moment I'll go with not worrying about it.

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/reader_writer.dart (right):

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/reader_writer.dart:238: abstract class ReaderWriter {
On 2013/01/25 01:19:59, John Messerly wrote:
> hmmm, I would expect ReaderWriter to do both reading and writing. Not sure
what
> other name to use though... "SerializerBase"? 

Changed it to ReaderOrWriter, which still isn't great, but at least immediately
gives the idea that it's some kind of odd union type rather than something which
both reads and writes.

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
File pkg/serialization/lib/src/serialization_rule.dart (right):

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/serialization_rule.dart:220: bool keysAreAllStrings =
state.keys.every((x) => x is String);
On 2013/01/25 01:19:59, John Messerly wrote:
> out of curiousity, why do we check all keys? Could we check each key
individual
> and preserve strings when possible?

I don't think that would work, at least not with the current state of the JSON
support. A lot of the point of retaining things in Map form is to make readable
JSON. But the JSON support requires any Maps in its arguments to have string
keys, and will fail otherwise. So if we see any non-string keys we need to turn
the whole thing into a List instead.

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/lib/src/...
pkg/serialization/lib/src/serialization_rule.dart:222:
keysAndValues(state).forEach(
On 2013/01/25 01:19:59, John Messerly wrote:
> I would have expected a return in both branches. Could we "return state;"
here?

Sure. The normal case for flatten() is that it destructively modifies the
existing state, and returning null is interpreted as just meaning to use the old
one. But consistently returning it in both cases probably makes the intention
clearer, so doing that.

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/test/ser...
File pkg/serialization/test/serialization_test.dart (right):

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/test/ser...
pkg/serialization/test/serialization_test.dart:51: (x) => x is BasicRule &&
x.type == reflect(p1).type);
On 2013/01/25 01:19:59, John Messerly wrote:
> Do type literals work now? if so could this be "x.type == Person"?

They work syntactically, but they return a Type, and there's no way to go
between a Type and a ClassMirror.

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/test/ser...
pkg/serialization/test/serialization_test.dart:390: var reader = s.newReader(new
SimpleJsonFormat(storeRoundTripInfo: true));
On 2013/01/25 01:19:59, John Messerly wrote:
> out of curiosity, would it work to use the same SimpleJsonFormat for both
reader
> and writer?

Yes. Changed one of these to re-use the same one and left the other creating a
new instance, just to cover the possibility that one or the other might fail at
some point.

https://codereview.chromium.org/11931030/diff/6001/pkg/serialization/test/ser...
pkg/serialization/test/serialization_test.dart:517: reader.data = new
List(10).mappedBy((x) => [sampleData]).toList();
On 2013/01/25 01:19:59, John Messerly wrote:
> Would it work to use "new List.filled(10, [sampleData])" here?

Yes, done.

[Jennifer Messerly, 2013-01-26 01:58:02]

lgtm