Some fixes to work better with the services framework

pkg/serialization/lib/src/format.dart

Index: pkg/serialization/lib/src/format.dart
diff --git a/pkg/serialization/lib/src/format.dart b/pkg/serialization/lib/src/format.dart
index c1d122e064bfc999e1d268fa0b3a91cd7172750b..439ae89b6ab0acd8a882fd07a421c53cb05e00de 100644
--- a/pkg/serialization/lib/src/format.dart
+++ b/pkg/serialization/lib/src/format.dart
@@ -52,23 +52,22 @@ class SimpleMapFormat extends Format {
    * This effectively defines a custom JSON serialization format, although
    * the details of the format vary depending which rules were used.
    */
-  String generateOutput(Writer w) {
+  generateOutput(Writer w) {

[justinfagnani, 2013/02/09 21:21:56]

Style nit: Why remove the type here? Since the method does return something, at
least Object would show that. Maybe the doc needs to be update where it says
"return it as a String"?

[Jennifer Messerly, 2013/02/10 00:20:35]

Style nit nit :)

If return type is "dynamic", we don't write "dynamic".

"dynamic" return type is more friendly than "Object". I agree about using
"Object" as a parameter though -- it says you aren't going to use any API beyond
object.

However this particular method could be declared to return Map<String, dynamic>.

[Alan Knight, 2013/02/11 23:38:42]

Good point, changed this to return Map<String, dynamic>. 

     var result = {
       "rules" : w.serializedRules(),
       "data" : w.states,
       "roots" : w._rootReferences()
     };
-    return json.stringify(result);
+    return result;
   }
 
   /**
-   * Read a [json] encoded string representing serialized data in this format
+   * Read a [json] compatible representation of serialized data in this format
    * and return the nested Map representation described in [generateOutput]. If
    * the data also includes rule definitions, then these will replace the rules
    * in the [Serialization] for [reader].
    */
-  Map<String, dynamic> read(String input, Reader reader) {
-    var topLevel = json.parse(input);
+  Map<String, dynamic> read(topLevel, Reader reader) {
     var ruleString = topLevel["rules"];
     reader.readRules(ruleString);
     return topLevel;
@@ -103,12 +102,20 @@ class SimpleJsonFormat extends Format {
   SimpleJsonFormat({this.storeRoundTripInfo : false});
 
   /**
-   * Generate output for this format from [w] and return it as a String which
-   * is the [json] representation of a nested Map structure.
+   * Generate output for this format from [w] and return it as
+   * the [json] representation of a nested Map structure.
    */
-  String generateOutput(Writer w) {
+  generateOutput(Writer w) {

[justinfagnani, 2013/02/09 21:21:56]

Map as the return type?

[Jennifer Messerly, 2013/02/10 00:20:35]

I don't think it necessarily returns a map if not selfDescribing or not
storeRoundTripInfo

[Alan Knight, 2013/02/11 23:38:42]

It doesn't necessarily return a Map, part of the changes in this CL are that the
SimpleJsonFormat maps simple types to themselves, so "abc" -> "abc" and 3 -> 3.
At least if not using the round trip info or selfDescribing. Makes the output
more compact and readable, but not very typeable.

     jsonify(w);
-    return json.stringify(w.stateForReference(w._rootReferences().first));
+    var root = w._rootReferences().first;
+    if (root is Reference) root = w.stateForReference(root);
+    if (w.selfDescribing && storeRoundTripInfo) {
+      root = {
+          "rules" : w.serializedRules(),

[Jennifer Messerly, 2013/02/08 04:04:38]

I think I usually just indent this +2, since it has matching end curly. or could
be one line?

[Alan Knight, 2013/02/11 23:38:42]

Because I made those be identifiers rather than literal strings I can't use a
literal map any more :-( So now it's cascaded assignments, which I think does
want four spaces 

+          "data" : root
+        };
+    }
+    return root;
   }
 
   /**
@@ -156,13 +163,18 @@ class SimpleJsonFormat extends Format {
    * converted into Reference objects. Note that if the data was not written
    * with [storeRoundTripInfo] true this will fail.
    */
-  Map<String, dynamic> read(String input, Reader r) {
-    var data = json.parse(input);
-    var result = {};
-    result["rules"] = null;
-    var ruleData =
-        new List(r.serialization.rules.length).map((x) => []).toList();
-    var top = recursivelyFixUp(data, r, ruleData);
+  Map<String, dynamic> read(data, Reader reader) {
+    var result = new Map();
+    bool looksLikeItHasOurExtraData = data is Map &&
+        data.containsKey("rules") && data.containsKey("data");
+    var rules = looksLikeItHasOurExtraData ? data["rules"] : null;
+    reader.readRules(rules);

[justinfagnani, 2013/02/09 21:21:56]

what happens here if "rules" is just some user data? Could the
serialization-related properties be made "private" with a '_' prefix?

[Jennifer Messerly, 2013/02/10 00:20:35]

This is a good idea.

We should go even further IMO. User should need to specify explicitly if the
format is "self describing JSON" or normal JSON. We shouldn't have any
possibility of incorrectly recognizing the data. "_" prefix is nice but still
not enough, IMO.

[Alan Knight, 2013/02/11 23:38:42]

OK, I changed this to just look at the "selfDescribing" property on the
serialization. Which I think should probably move to the format, but will do
that in a different CL, so added a TODO.

That makes this a little bit more fragile if the user saves it one way and tries
to read it the other, but simplifies the code. So I added an explicit check that

throws if it looks like it was written without selfDescribing set and is being
read with it.

Also changed these to use _rules, _data, _root for this format, and put them
into constants. I only did it for this format because this is the only one that
has the possibility of mixing user data with private serialization data, as
we're trying to write both plain JSON and JSON with our annotations. And it
minimized the extent of the change.

+    var ruleData = new List(reader.serialization.rules.length).
+        map((x) => []).toList();
+    // If our result was a map with rules and data, get the data part. Otherwise
+    // assume that the whole thing is the data.
+    var actualData = (rules == null) ? data : data["data"];
+    var top = recursivelyFixUp(actualData, reader, ruleData);
     result["data"] = ruleData;
     result["roots"] = [top];
     return result;
@@ -171,20 +183,28 @@ class SimpleJsonFormat extends Format {
   /**
    * Convert nested references in [data] into [Reference] objects.
    */
-  recursivelyFixUp(data, Reader r, List result) {
+  recursivelyFixUp(input, Reader r, List result) {
+    var data = input;
     if (isPrimitive(data)) {
       result[r._primitiveRule().number].add(data);
       return data;
     }
     var ruleNumber;
+    // If we've added the rule number on as the last item in a list we have
+    // to get rid of it or it will be interpreted as extra data. For a map
+    // the library will be ok, but we need to get rid of the extra key before
+    // the data is shown to the user, so we destructively modify.
     if (data is List) {
-      ruleNumber = data.removeLast();
+      ruleNumber = data.last;
+      data = data.take(data.length -1);
     } else if (data is Map) {
       ruleNumber = data.remove(ruleIdentifier);
     } else {
       throw new SerializationException("Invalid data format");
     }
-    var newData = mapValues(data, (x) => recursivelyFixUp(x, r, result));
+    // Do not use mappedBy or other lazy operations for this. They do not play
+    // well with a function that destructively modifies its arguments.
+    var newData = mapValues(data, (each) => recursivelyFixUp(each, r, result));
     result[ruleNumber].add(newData);
     return new Reference(r, ruleNumber, result[ruleNumber].length - 1);
   }