Make input/output formats pluggable, adapt to new libraries

Make output formats pluggable, adapt to new libraries

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

[Alan Knight, 2013-01-09 19:13:48]

This introduces pluggable Format objects. It retains the two formats we had
before, and introduces one that does more "normal" JSON.

This changes, or at least defines a bit more clearly, the contract between rules
and reader/writer. Rather than saying the rule can store the data absolutely any
way it wants, we say that it stores it as either a primitive (num, bool, String,
null), a List, or a Map, and recursive structures built from same. Then the
format needs to be able to deal with those forms.

A bunch of code is deleted or moved to a Format object. Some things are
generalized (e.g. writeLengthInFlatFormat is renamed to
hasVariableLengthEntries). Documentation is updated, including some that wasn't
updated from previous changes.

I'm a bit confused why _LazyList appears in this CL. I thought that was part of
an earlier commit. But it doesn't seem to have been in there, so fine.

There are also some changes in here related to using the new libraries. There
was a basic adaptation done, but I change it a bit, and try to avoid just
converting everything toList() immediately.

Sorry for the size. I hope this is the last of the really large CLs and that
things can be more fine-grained going forwards.

[Jennifer Messerly, 2013-01-11 02:21:53]

Sorry this took me so long.
Changes look great overall, just a few questions about naming and such

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/seriali...
File pkg/serialization/lib/serialization.dart (right):

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/seriali...
pkg/serialization/lib/serialization.dart:56: *        bool appliesTo(instance,
Writer w) => instance.runtimeType == Address;
"instance is Address"?

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/seriali...
pkg/serialization/lib/serialization.dart:156: * [selfDescribing] variable is
true by default, but the SimpleJSONFormat does
SimpleJsonFormat?

I think we do camel case (e.g. innerHTML became innerHtml)

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/seriali...
pkg/serialization/lib/serialization.dart:161: * Named Objects
like these new sections!

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
File pkg/serialization/lib/src/format.dart (right):

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:56: var result = new Map();
consider map literal since keys are all const strings?

var result = {
  'rules': w.serializedRules(),
  'data': w.states,
  'roots': w._rootReferences()
};

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:69: Map<String, dynamic>read(String input,
Reader reader) {
space after >

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:95: final bool storeAdditionalData;
this name is good, but I wonder if we could make it more descriptive? e.g.
"storeRoundTripInfo" or "storeDeserializationData" or "storeRuleMetadata",
something like that? something that adds more detail about what the additional
data is.

the doc comment is great though :)

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:97:
SimpleJSONFormat([this.storeAdditionalData = false]);
make this a named arg?

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:142: if (value is Reference) map[key] =
w.stateForReference(value);
-2 indent

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:153: Map<String, dynamic>read(String
input, Reader r) {
also space here after >

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/rea...
File pkg/serialization/lib/src/reader_writer.dart (right):

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/rea...
pkg/serialization/lib/src/reader_writer.dart:131: bool get
shouldUseReferencesForPrimitives
wonder if both of these could be named "useReferencesForPrimitives"?

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/ser...
File pkg/serialization/lib/src/serialization_rule.dart (right):

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:418: List _raw;
final?

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/test/serial...
File pkg/serialization/test/serialization_test.dart (right):

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/test/serial...
pkg/serialization/test/serialization_test.dart:325: //  test("Straight JSON
format", () {
what's the status of this test?

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/test/serial...
pkg/serialization/test/serialization_test.dart:341: var reconstituted =
json.parse(out);
it would be interesting to see the full JSON map here, e.g.

expected(json.parse(out), {
  'name': 'Alice',
  'rank': null,
  'serialNumber': null,
  'address': { 'street': 'N 34th', 'city': 'Seatte',
               'state': null, 'zip', null, ... }
  ...
});

I think matchers can compare maps?

[Alan Knight, 2013-01-11 19:18:11]

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/seriali...
File pkg/serialization/lib/serialization.dart (right):

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/seriali...
pkg/serialization/lib/serialization.dart:56: *        bool appliesTo(instance,
Writer w) => instance.runtimeType == Address;
On 2013/01/11 02:21:53, John Messerly wrote:
> "instance is Address"?

I actually did that on purpose, because if we have subclasses of Address then an
"is" test would also pick those up, and I think we need to have separate rules
for each subclass. But if I'm doing that I should explain it.

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/seriali...
pkg/serialization/lib/serialization.dart:156: * [selfDescribing] variable is
true by default, but the SimpleJSONFormat does
On 2013/01/11 02:21:53, John Messerly wrote:
> SimpleJsonFormat?
> 
> I think we do camel case (e.g. innerHTML became innerHtml)

Done.

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
File pkg/serialization/lib/src/format.dart (right):

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:56: var result = new Map();
On 2013/01/11 02:21:53, John Messerly wrote:
> consider map literal since keys are all const strings?
> 
> var result = {
>   'rules': w.serializedRules(),
>   'data': w.states,
>   'roots': w._rootReferences()
> };

I forget I can do that. Done.

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:69: Map<String, dynamic>read(String input,
Reader reader) {
On 2013/01/11 02:21:53, John Messerly wrote:
> space after >

Done.

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:95: final bool storeAdditionalData;
On 2013/01/11 02:21:53, John Messerly wrote:
> this name is good, but I wonder if we could make it more descriptive? e.g.
> "storeRoundTripInfo" or "storeDeserializationData" or "storeRuleMetadata",
> something like that? something that adds more detail about what the additional
> data is.
> 
> the doc comment is great though :)

Done.

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:97:
SimpleJSONFormat([this.storeAdditionalData = false]);
On 2013/01/11 02:21:53, John Messerly wrote:
> make this a named arg?

Done.

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:142: if (value is Reference) map[key] =
w.stateForReference(value);
On 2013/01/11 02:21:53, John Messerly wrote:
> -2 indent

Done.

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:153: Map<String, dynamic>read(String
input, Reader r) {
On 2013/01/11 02:21:53, John Messerly wrote:
> also space here after >

Done.

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/ser...
File pkg/serialization/lib/src/serialization_rule.dart (right):

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:418: List _raw;
On 2013/01/11 02:21:53, John Messerly wrote:
> final?

Done.

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:418: List _raw;
On 2013/01/11 02:21:53, John Messerly wrote:
> final?

Done.

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/test/serial...
File pkg/serialization/test/serialization_test.dart (right):

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/test/serial...
pkg/serialization/test/serialization_test.dart:325: //  test("Straight JSON
format", () {
On 2013/01/11 02:21:53, John Messerly wrote:
> what's the status of this test?

Oops. Restored.

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/test/serial...
pkg/serialization/test/serialization_test.dart:341: var reconstituted =
json.parse(out);
On 2013/01/11 02:21:53, John Messerly wrote:
> it would be interesting to see the full JSON map here, e.g.
> 
> expected(json.parse(out), {
>   'name': 'Alice',
>   'rank': null,
>   'serialNumber': null,
>   'address': { 'street': 'N 34th', 'city': 'Seatte',
>                'state': null, 'zip', null, ... }
>   ...
> });
> 
> I think matchers can compare maps?

Nice. Done.

[Jennifer Messerly, 2013-01-11 21:54:25]

lgtm!

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/seriali...
File pkg/serialization/lib/serialization.dart (right):

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/seriali...
pkg/serialization/lib/serialization.dart:56: *        bool appliesTo(instance,
Writer w) => instance.runtimeType == Address;
On 2013/01/11 19:18:11, Alan Knight wrote:
> On 2013/01/11 02:21:53, John Messerly wrote:
> > "instance is Address"?
> 
> I actually did that on purpose, because if we have subclasses of Address then
an
> "is" test would also pick those up, and I think we need to have separate rules
> for each subclass. But if I'm doing that I should explain it.

Yeah, comment would be nice. That makes sense.
Do you need to check for "instance != null" here, or is that handled before we
get to appliesTo?

fwiw, this is one of the first times I've seen an exact type test used in Dart
code. It's neat it can be written so easily.

[Alan Knight, 2013-01-11 22:35:00]

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/seriali...
File pkg/serialization/lib/serialization.dart (right):

https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/seriali...
pkg/serialization/lib/serialization.dart:56: *        bool appliesTo(instance,
Writer w) => instance.runtimeType == Address;
On 2013/01/11 21:54:25, John Messerly wrote:
> On 2013/01/11 19:18:11, Alan Knight wrote:
> > On 2013/01/11 02:21:53, John Messerly wrote:
> > > "instance is Address"?
> > 
> > I actually did that on purpose, because if we have subclasses of Address
then
> an
> > "is" test would also pick those up, and I think we need to have separate
rules
> > for each subclass. But if I'm doing that I should explain it.
> 
> Yeah, comment would be nice. That makes sense.
> Do you need to check for "instance != null" here, or is that handled before we
> get to appliesTo?
> 
> fwiw, this is one of the first times I've seen an exact type test used in Dart
> code. It's neat it can be written so easily.

null will  get caught by the PrimitiveRule that comes first. Although it appears
that null.runtimeType works, and returns the type Null.

[Jennifer Messerly, 2013-01-12 01:53:08]

On 2013/01/11 22:35:00, Alan Knight wrote:
>
https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/seriali...
> File pkg/serialization/lib/serialization.dart (right):
> 
>
https://codereview.chromium.org/11820032/diff/1/pkg/serialization/lib/seriali...
> pkg/serialization/lib/serialization.dart:56: *        bool appliesTo(instance,
> Writer w) => instance.runtimeType == Address;
> On 2013/01/11 21:54:25, John Messerly wrote:
> > On 2013/01/11 19:18:11, Alan Knight wrote:
> > > On 2013/01/11 02:21:53, John Messerly wrote:
> > > > "instance is Address"?
> > > 
> > > I actually did that on purpose, because if we have subclasses of Address
> then
> > an
> > > "is" test would also pick those up, and I think we need to have separate
> rules
> > > for each subclass. But if I'm doing that I should explain it.
> > 
> > Yeah, comment would be nice. That makes sense.
> > Do you need to check for "instance != null" here, or is that handled before
we
> > get to appliesTo?
> > 
> > fwiw, this is one of the first times I've seen an exact type test used in
Dart
> > code. It's neat it can be written so easily.
> 
> null will  get caught by the PrimitiveRule that comes first. Although it
appears
> that null.runtimeType works, and returns the type Null.

awesome!