Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11293283:
Initial version of a serialization framework (Closed)
|
Created:
8 years, 1 month ago by Alan Knight Modified:
8 years ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionInitial version of a serialization framework
Committed: https://code.google.com/p/dart/source/detail?r=15796
Patch Set 1 #Patch Set 2 : #
Total comments: 63
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 103
Patch Set 7 : #Patch Set 8 : #
Total comments: 2
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #Messages
Total messages: 11 (0 generated)
A first cut at serialization. Still a great many issues, but will do the basics. There's also a brief design overview doc that I'll share with you.
Only part way through so far, but figured I'd send comments so far. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... File pkg/serialization/lib/serialization.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:6: * This provides a general-purpose serialization facility for Dart objects. A btw, this comment is lovely. So much content! https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:9: * var address = new Address(); general formatting: I think this might be easier to read with a newline before and after each code block. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:18: * back a String which is a JSON representation of the state of it and related this could link to JSON, e.g. [JSON] and add an import below: import 'dart:json' show JSON; // for doc comment https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:50: * created object. At the moment, however, this is more likely to cause problems This is an intriguing sentence. It makes me want to know more about the problem with cycles. Could you elaborate, maybe with another sentence or two? https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:57: * In some cases a non-constructor field cannot be properly set using field suggestion: remove "properly" ... it is too easy to read as "property", and I think the wording is clear without it https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:66: * currently two variations. perhaps just say "There are two variations", then describe them, and wait for the "we expect to generalize" to introduce the possibility of a third variation. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:77: * Both representations as primarily intended as proofs of concept for different as primarily -> are primarily https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:81: * To read objects, the corresponding methods are [read] and [readFlat]. I think it would be helpful to use the markdown Headers to create a section for "reading" and "writing", and perhaps one at the start for "setup". Something like <intro paragraph> Setup === <discuss APIs for constructing serializer> Reading ===== <discuss APIs for reading> Writing ===== <discuss APIs for reading> that way someone that cares about reading or writing can visually jump to the right place. (I can imagine the Setup section eventually splitting into "Mirrors" "Manual" if it got big enough) https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:103: * the simple name of classes we are interested in to a ClassMirror. This can would be nice to link to classmirror, if possible https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:110: looks like extra newline here https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:127: class Serialization { random idea, could we call this Serializer? https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:136: * When reading, we may need to resolve references to existing objects in The part about class mirrors is not totally clear to me. why can't we construct class mirrors? What kind of object is the value in that case? an example would help https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:141: Map<String, dynamic> externalObjects = new Map(); if you use "{}" instead of new Map, I think the key type will be constrained to string and checked in checked mode https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:162: Serialization.noDefaultRules() { } Serialization.blank? https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:189: someInstanceThatWeHaveToPassInBecauseWeCantUseLiteralTypes, haha :) as funny as this is, I would give it a normal param name, like "instanceForType" so it appears sane in user facing docs https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:297: // consider having the arbitrary predicates be secondary to an initial this seems reasonable to me. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:311: return [newRule]; return [addRuleFor(target)]; ? https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:316: (x) => !(x != applicable[0] && x.mustBePrimary)); probably clearer as !x.mustBePrimary || x == applicable[0] out of curiosity, why is the first element special? https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... File pkg/serialization/lib/src/basic_rule.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/basic_rule.dart:25: ClassMirror type; can anything bad happen if this is mutated after construction? https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/basic_rule.dart:36: * representation is much easier to debug. The default is to use lists. would maps be an easier thing to use for common serialization cases? I'm thinking of JSON, (and HTML/XML) which is less compact but seems quite popular. It seems that debuggable output is usually preferred in serialization for small-medium ish data sizes. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/basic_rule.dart:306: // is not the inverse of regular. either way, doc comments for these two properties would help :) https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/basic_rule.dart:465: figureOutFields() { void? https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... File pkg/serialization/lib/src/mirrors_helpers.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/mirrors_helpers.dart:11: // and the number of times we have to be told that mirrors aren't finished yet. Did you know: mirrors aren't finished yet? ;) https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... File pkg/serialization/lib/src/polyfill_identity_set.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/polyfill_identity_set.dart:13: library identity_set; I'm going to assume this is basically a copy+paste from hash_map_set and not look at it too closely... https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/polyfill_identity_set.dart:99: } else if (existingKey === key) { fwiw, I think === is going away in favor of identical(x, y) https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/p... File pkg/serialization/pubspec.yaml (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/p... pkg/serialization/pubspec.yaml:1: name: Serialization Name should be lower case I think (with_underscores_if_needed). I wonder if we can make the name shorter, maybe "serialize"? for publishing to hosted, we'll want author: "Dart Team <misc@dartlang.org>" https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/t... File pkg/serialization/test/polyfill_identity_set_test.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/t... pkg/serialization/test/polyfill_identity_set_test.dart:13: import '../../unittest/lib/unittest.dart'; can this be imported with package:? https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/t... pkg/serialization/test/polyfill_identity_set_test.dart:14: import '../lib/src/polyfill_identity_set.dart'; likewise, using the self-link? 'package:serialization/polyfill_identity_set.dart' https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/t... File pkg/serialization/test/serialization_test.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/t... pkg/serialization/test/serialization_test.dart:21: // TODO(alanknight): Switch these to use literal types. could you clarify "literal types" https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/t... pkg/serialization/test/serialization_test.dart:77: var list = [5,4,3,2,1]; i think the style guide has spaces here https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/t... pkg/serialization/test/serialization_test.dart:241: test('eating your own tail', () { haha, awesome :)
John suggests renaming the package from serialization to serialize. That sounds good to me. What do others think? https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... File pkg/serialization/lib/serialization.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:6: * This provides a general-purpose serialization facility for Dart objects. A On 2012/11/15 08:02:14, John Messerly wrote: > btw, this comment is lovely. So much content! Thanks. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:9: * var address = new Address(); On 2012/11/15 08:02:14, John Messerly wrote: > general formatting: I think this might be easier to read with a newline before > and after each code block. Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:18: * back a String which is a JSON representation of the state of it and related On 2012/11/15 08:02:14, John Messerly wrote: > this could link to JSON, e.g. [JSON] > > and add an import below: > > import 'dart:json' show JSON; // for doc comment I'm not quite sure what you mean by the second part. The user doesn't actually have to import JSON in order to use this, they just get back a string that they can give back to a read() method. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:50: * created object. At the moment, however, this is more likely to cause problems On 2012/11/15 08:02:14, John Messerly wrote: > This is an intriguing sentence. It makes me want to know more about the problem > with cycles. Could you elaborate, maybe with another sentence or two? Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:57: * In some cases a non-constructor field cannot be properly set using field On 2012/11/15 08:02:14, John Messerly wrote: > suggestion: remove "properly" ... it is too easy to read as "property", and I > think the wording is clear without it Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:66: * currently two variations. On 2012/11/15 08:02:14, John Messerly wrote: > perhaps just say "There are two variations", then describe them, and wait for > the "we expect to generalize" to introduce the possibility of a third variation. Done. I think I keep wanting to apologize for the state of the output format handling :-) https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:77: * Both representations as primarily intended as proofs of concept for different On 2012/11/15 08:02:14, John Messerly wrote: > as primarily -> are primarily Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:81: * To read objects, the corresponding methods are [read] and [readFlat]. On 2012/11/15 08:02:14, John Messerly wrote: > I think it would be helpful to use the markdown Headers to create a section for > "reading" and "writing", and perhaps one at the start for "setup". Something > like > > <intro paragraph> > > Setup > === > > <discuss APIs for constructing serializer> > > Reading > ===== > > <discuss APIs for reading> > > > Writing > ===== > > <discuss APIs for reading> > > that way someone that cares about reading or writing can visually jump to the > right place. > > > (I can imagine the Setup section eventually splitting into "Mirrors" "Manual" if > it got big enough) Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:103: * the simple name of classes we are interested in to a ClassMirror. This can On 2012/11/15 08:02:14, John Messerly wrote: > would be nice to link to classmirror, if possible Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:110: On 2012/11/15 08:02:14, John Messerly wrote: > looks like extra newline here Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:127: class Serialization { On 2012/11/15 08:02:14, John Messerly wrote: > random idea, could we call this Serializer? I really prefer calling things nouns rather than verbs if I can. I really didn't have a better name for Reader and Writer, but I like Serialization much better than Serializer. Also, it doesn't really do the work, it's more of a configuration object. And grammatically - you define a serialization, and if you want to read objects differently on the client side then you need a different serialization on each side. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:136: * When reading, we may need to resolve references to existing objects in On 2012/11/15 08:02:14, John Messerly wrote: > The part about class mirrors is not totally clear to me. why can't we construct > class mirrors? What kind of object is the value in that case? an example would > help We can't construct them because they're an abstract class with no constructor. The only way I know of to get a ClassMirror that actually works right now is to have an instance and do reflect(anInstance).type. In theory, reflect(anInstance.runtimeType) or reflect(Object) should work, but right now the first one gives you an InstanceMirror on Type, and the second one doesn't work. But rewrote this to give a more meaningful domain example. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:141: Map<String, dynamic> externalObjects = new Map(); On 2012/11/15 08:02:14, John Messerly wrote: > if you use "{}" instead of new Map, I think the key type will be constrained to > string and checked in checked mode Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:162: Serialization.noDefaultRules() { } On 2012/11/15 08:02:14, John Messerly wrote: > Serialization.blank? Yes, much better. Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:189: someInstanceThatWeHaveToPassInBecauseWeCantUseLiteralTypes, On 2012/11/15 08:02:14, John Messerly wrote: > haha :) > > as funny as this is, I would give it a normal param name, like "instanceForType" > so it appears sane in user facing docs Awww. Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:297: // consider having the arbitrary predicates be secondary to an initial On 2012/11/15 08:02:14, John Messerly wrote: > this seems reasonable to me. OK https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:311: return [newRule]; On 2012/11/15 08:02:14, John Messerly wrote: > return [addRuleFor(target)]; ? Done. I often find myself pulling stuff apart into multiple lines like that so the debugger will let me see it better. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:316: (x) => !(x != applicable[0] && x.mustBePrimary)); On 2012/11/15 08:02:14, John Messerly wrote: > probably clearer as > > !x.mustBePrimary || x == applicable[0] > > out of curiosity, why is the first element special? If something has mustBePrimary, then it has to be the first rule. Otherwise it's not primary. That whole business is quite ugly, as it forces an order dependency in the rules, and I'd like to get rid of it. Right now it exists because of the ListRule/EssentialListRule distinction, where both apply to Lists, but only one should be used. Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... File pkg/serialization/lib/src/basic_rule.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/basic_rule.dart:25: ClassMirror type; On 2012/11/15 08:02:14, John Messerly wrote: > can anything bad happen if this is mutated after construction? I think everything would be ok, but there's also no reasonable use case I can think of where you'd want to, so made it final. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/basic_rule.dart:36: * representation is much easier to debug. The default is to use lists. On 2012/11/15 08:02:14, John Messerly wrote: > would maps be an easier thing to use for common serialization cases? > I'm thinking of JSON, (and HTML/XML) which is less compact but seems quite > popular. It seems that debuggable output is usually preferred in serialization > for small-medium ish data sizes. It might be, I'm not sure. Changed the text to say human-readable, which probably better conveys that. Are you suggesting we should change the default? https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/basic_rule.dart:306: // is not the inverse of regular. On 2012/11/15 08:02:14, John Messerly wrote: > either way, doc comments for these two properties would help :) Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/basic_rule.dart:465: figureOutFields() { On 2012/11/15 08:02:14, John Messerly wrote: > void? Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... File pkg/serialization/lib/src/mirrors_helpers.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/mirrors_helpers.dart:11: // and the number of times we have to be told that mirrors aren't finished yet. On 2012/11/15 08:02:14, John Messerly wrote: > Did you know: > mirrors aren't finished yet? ;) :-) https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... File pkg/serialization/lib/src/polyfill_identity_set.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/polyfill_identity_set.dart:13: library identity_set; On 2012/11/15 08:02:14, John Messerly wrote: > I'm going to assume this is basically a copy+paste from hash_map_set and not > look at it too closely... Yes, it's a cut and paste with two or three very small changes. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/src/polyfill_identity_set.dart:99: } else if (existingKey === key) { On 2012/11/15 08:02:14, John Messerly wrote: > fwiw, I think === is going away in favor of identical(x, y) I think I'll hope I can delete this code entirely before that becomes mandatory. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/p... File pkg/serialization/pubspec.yaml (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/p... pkg/serialization/pubspec.yaml:1: name: Serialization On 2012/11/15 08:02:14, John Messerly wrote: > Name should be lower case I think (with_underscores_if_needed). > I wonder if we can make the name shorter, maybe "serialize"? > > for publishing to hosted, we'll want > author: "Dart Team <misc@dartlang.org>" Done, except for the name change. I like it, but will take a poll. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/t... File pkg/serialization/test/polyfill_identity_set_test.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/t... pkg/serialization/test/polyfill_identity_set_test.dart:13: import '../../unittest/lib/unittest.dart'; On 2012/11/15 08:02:14, John Messerly wrote: > can this be imported with package:? As I understand it, not until we've got the situation with running pub on the test bots straightened out. The intl imports are the same way. It's unfortunate, but I hope it'll be fixed soon. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/t... File pkg/serialization/test/serialization_test.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/t... pkg/serialization/test/serialization_test.dart:21: // TODO(alanknight): Switch these to use literal types. On 2012/11/15 08:02:14, John Messerly wrote: > could you clarify "literal types" Class literals. Added the issue number. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/t... pkg/serialization/test/serialization_test.dart:77: var list = [5,4,3,2,1]; On 2012/11/15 08:02:14, John Messerly wrote: > i think the style guide has spaces here Done. https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/t... pkg/serialization/test/serialization_test.dart:77: var list = [5,4,3,2,1]; On 2012/11/15 08:02:14, John Messerly wrote: > i think the style guide has spaces here Done.
Anybody besides John have comments?
Here is round #2 of my comments :) https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... File pkg/serialization/lib/src/reader_writer.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:40: Map<Object, Reference> references = new IdentityMap<Object, Reference>(); make this final? likewise for "states" and "trace" https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:59: * for multiple different Readers/Writers. Can I reuse a Writer? I see a lot of methods that look stateless, such as "toMaps", but it's not clear what the affect would be if I called it multiple times. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:87: shouldUseReferencesForPrimitives = true; Should this be reset at the end? Can this variable be private? Ideally calling writeFlat would not have side effects. In particular, if I can "writeFlat" can I call "write" later, or would I need to reset this flag to false? https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:95: * Write an incredibly cheesy flat format, just to verify that we can handle Ideally we could make this comment a bit more user-facing. It sounds great for a fellow developer reading the code, but as a user calling toFlatFormat I'd want more information. Perhaps something along the lines of: /** * Writes to a simple flat format. The details of this format have not * been finalized and may change in the future. For now this produces a * List that contains null, ints, strings, or nested Lists. * * Consider using [toStringFormat] or [toMaps] instead for most use cases. */ https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:114: var flatData = new List(); personally I tend to use "[]" instead of "new List()" https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:125: * Given that we have fully populated the list of [states], and more Should we check these preconditions in code? Since this type+method is public, I'm wondering if a user could get messed up by calling methods in the wrong order and violating the invariants. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:163: new Reference(this, rule.number, _nextObjectNumberFor(rule))); +2 indent https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:225: /** Return the serialized data in string format. Currently hard-coded to newline after /** here and in toJSON https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:235: String toJSON(data) { I'm not sure this needs a function, perhaps just inline the JSON.stringify call? https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:247: * This effectively defines a custom JSON serialization format, although Is there a way I can use serialization with existing JSON formats, which don't have rules/data/roots? https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:270: roots.map((x) => _referenceFor(x)); _rootReferences(roots) => roots.map(_referenceFor); https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:359: get rules => serialization.rules; these could use doc comments+type annotation, if they are public facing https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:383: return roots.map((x) => inflateReference(x)); could be "return roots.map(inflateReference)" ? https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:438: => read(input, externals)[0]; for consistency put => on previous line https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:529: if (anObject is Map && (anObject["__Ref"] == true)) { parens not needed for the == expr https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:544: class Sentinel { Might want to make the type library private, so no one else can type test or create a Sentinel. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:564: Queue queue = new Queue(); final? likewise for roots? https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:634: "object" : objectNumber}; I think we usually +2 more indent here (which we inherited from Java style guide). alternatively, this style works too: toJson() => { "__Ref" : true, "rule" : ruleNumber, "object" : objectNumber }; https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... File pkg/serialization/lib/src/serialization_helpers.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:7: * serialiation. Some or all of them might be removable with changes to the perhaps reword second sentence? Some or all of these will be removed once the functionality is available in the core library. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:19: /** Concatenate two lists. Handle the case where one or both might be null. */ this seems to me like it should exist in core libraries somewhere? if so, do you mind filing a bug? with a bug+TODO it will be easier to track if we can remove it later. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:20: append(List a, List b) { return type List https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:22: if (b == null) return []; else return new List.from(b); should use curlies here: http://www.dartlang.org/articles/style-guide/#do-use-curly-braces-for-all-flo... https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:56: class MapLikeIterable { should this iterate a key-value pairs? then it could implement Collection for real. in C#, Dictionary implements Enumerable<KeyValuePair<K,V>>, so all of the list-related functionality "just works" on pairs. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:62: if (collection is Map) { this seems to me like there should be two subtypes, one for when Collection is a Map, and another if it is a generic iterable. Generally lots of type tests imply polymorphism would be a cleaner solution. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:63: collection.forEach(f); not sure how I feel about forEach and iterator behaving differently. iterator is iterating over values but forEach is iterating over pairs... https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:125: class ListLikeIterable { I'm not seeing the point of this class. If collection is! Map, then all the methods simply forward. If the collection is a Map, then it behaves like MapLikeIterable, except for forEach... Personally, I would try to redesign these two types (ListLike and MapLike) and see if: * they can both be defined as "implements Collection<T>" (or List<T>) and use generics * they can avoid wrapping normal lists https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:132: collection.forEach((key, value) => f(value)); is this just collection.values.forEach(f)? https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... File pkg/serialization/lib/src/serialization_rule.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_rule.dart:15: int number; Is this the index of the rule? I can't recall how this was used, but it would be nice if this could be passed in at the appropriate places. That would avoid duplication and potential bugs around inconsistency (if number becomes different from the rule's index). https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_rule.dart:72: inflateEssential(state, reader); Apologies if I mentioned this before -- I find "essential" terminology a bit confusing. I had never heard this term before. Perhaps: "inflateConstrutorArgs" and "inflateSetters" ? I think typical developers would be more likely to guess the meaning with a name like that. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_rule.dart:261: return (object is num) || (object is String) || (object is bool); personally I wouldn't use parens here. C++ precedence rules can be confusing, but it's generally understood that || and && are lower than comparisons, and that tends to hold across languages https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_rule.dart:280: Type type; final? https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_rule.dart:347: inflateNonEssential(object, _, Reader r) {} seems unusual to see "_" in the middle of a method declaration. Perhaps just use "(state, object, Reader r)" as above https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... File pkg/serialization/test/polyfill_identity_set_test.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/test/polyfill_identity_set_test.dart:13: import '../../unittest/lib/unittest.dart'; use "package:" here if you can (maybe doesn't work yet?) https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... File pkg/serialization/test/serialization_test.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/test/serialization_test.dart:399: (map, object) { object personally I'd put "object" on next line, and only indent the block by 2: http://www.dartlang.org/articles/style-guide/#do-indent-blocks-with-two-spaces https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/test/serialization_test.dart:463: return rules.map( (x) => x.extractState(object, doNothing)); looks like an extra space here
https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... File pkg/serialization/lib/serialization.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:18: * back a String which is a JSON representation of the state of it and related On 2012/11/15 20:51:03, Alan Knight wrote: > On 2012/11/15 08:02:14, John Messerly wrote: > > this could link to JSON, e.g. [JSON] > > > > and add an import below: > > > > import 'dart:json' show JSON; // for doc comment > > I'm not quite sure what you mean by the second part. The user doesn't actually > have to import JSON in order to use this, they just get back a string that they > can give back to a read() method. > > Oh I meant that you need to add "import 'dart:json' show JSON;" below in this very file. Otherwise DartDoc doesn't know what [JSON] is and it will render as plain text.
Hey Alan, Quick first pass mainly on the library dartdoc and APIs, naming etc. I'll get into code tomorrow. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... File pkg/serialization/lib/serialization.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:18: * ..addRuleFor(address); I'm confused by this line here: does it att a rule for the instance address, or the class Address? If it's the class, consider using Type as the parameter type and address.runtimeType in the example code for now. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:21: * This creates a new serialization and adds a rule for address objects. Right Why is it called "serialization" and not "serializer". I would expect that a serialization is the result of serializing some object. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:31: * ..addRuleFor(address, "Rule" as a name here doesn't seem ideal to me. Every other time I've encountered this concept it's usually been named "custom serializer" or something similar. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:36: * This rule still uses reflection to access the fields, but not to calculate Is there a way to clarify the phrase "not to calculate them"? maybe "not determine which fields to serialize" https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:54: * new ClosureToMapRule(anAddress.runtimeType, When using many closures together I find it cleaner to just define a class rather than to pass them all to a constructor. As a writer of rules, I'd rather write: class AddressRule extends Rule<Address> { final Type type = anAddress.type; // fix when literals are supported Map toMap(a) => {"number" : a.number, "street" : a.street, "city" : a.city}; Address fromMap(Map m) => new Address.create(m["number"], m["street"]); void fillIn(Address a, Map m) => a.city = m["city"]; } var serialization = new Serialization()..addRule(new AddressRule()); https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:57: * Note that there are three different functions provided. The first one "first one" -> "addressToMap" https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:59: * map. The second one creates a new address using a map like the one returned "second one" -> "createAddress" https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:61: * created object. At the moment, however, this is more likely to cause problems Consider breaking this paragraph up, maybe right before "At the moment," https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:74: * It is possible to give constructor fields values that aren't field names. If I don't understand this paragraph. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:84: * s..addRuleFor(fooHolderInstance).specialTreatmentFor("foo", I feel like "specialTreatementFor" is pretty long and doesn't denote that the treatment is for a field, which is important because the types don't indicate that either. How about something like handleField()? https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:111: * List input = serialization.readFlat(aList); Does this return a List of Lists? From the description of writeFlat above it sounds like a single object can be written to a list, so multiple objects would be written to nested lists. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:113: * There is also a convenience method for the case of reading a single object. Are there corresponding methods to write multiple objects? https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:116: * Object result = serialization.readOneFlat(aString); For multiple objects it would be great to have the equivalent of Java's ObjectInputStream and ObjectOutputStream. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:128: * When reading, some object references should not be serialized, but should be Why wouldn't this be done with a custom rule for that type? The rule can serialize the external object with the info needed to resolve it on the receiver, and the receiver and then resolve it however it needs to. This could be done with mirrors or without depending on the situation/environment. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:143: import 'src/mirrors_helpers.dart'; I think we're supposed to use package: urls even for internal imports. Maybe double check w/ someone in SEA though. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:145: //import 'src/polyfill_identity_set.dart'; remove https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:221: // TODO(alanknight): Take a type rather than an instance. Issue 6282. Can you still take a type and require that callers use .runtimeType() to get one?
https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... File pkg/serialization/lib/serialization.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/2001/pkg/serialization/l... pkg/serialization/lib/serialization.dart:18: * back a String which is a JSON representation of the state of it and related On 2012/11/20 01:23:31, John Messerly wrote: > On 2012/11/15 20:51:03, Alan Knight wrote: > > On 2012/11/15 08:02:14, John Messerly wrote: > > > this could link to JSON, e.g. [JSON] > > > > > > and add an import below: > > > > > > import 'dart:json' show JSON; // for doc comment > > > > I'm not quite sure what you mean by the second part. The user doesn't actually > > have to import JSON in order to use this, they just get back a string that > they > > can give back to a read() method. > > > > > > Oh I meant that you need to add "import 'dart:json' show JSON;" below in this > very file. Otherwise DartDoc doesn't know what [JSON] is and it will render as > plain text. Hmm, even when I do that, it still seems to show as plain text. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... File pkg/serialization/lib/src/reader_writer.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:40: Map<Object, Reference> references = new IdentityMap<Object, Reference>(); On 2012/11/20 01:20:47, John Messerly wrote: > make this final? likewise for "states" and "trace" Done for the first two. I don't think trace can be, but I think I'm going to fold what little of Trace is left into Writer in the next round anyway. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:59: * for multiple different Readers/Writers. On 2012/11/20 01:20:47, John Messerly wrote: > Can I reuse a Writer? I see a lot of methods that look stateless, such as > "toMaps", but it's not clear what the affect would be if I called it multiple > times. Right now you can't, because it holds onto state in [references] and [states]. You could probably re-use it if you cleared out those two, but I haven't tried it. My general thinking was that it's one-shot, representing a single write, and the thing you re-use is the Serialization. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:87: shouldUseReferencesForPrimitives = true; On 2012/11/20 01:20:47, John Messerly wrote: > Should this be reset at the end? Can this variable be private? > > Ideally calling writeFlat would not have side effects. In particular, if I can > "writeFlat" can I call "write" later, or would I need to reset this flag to > false? As above, definitely won't work right now because it would have all the objects twice. It might be made to, but I'm not sure if that's the right thing to do or not. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:95: * Write an incredibly cheesy flat format, just to verify that we can handle On 2012/11/20 01:20:47, John Messerly wrote: > Ideally we could make this comment a bit more user-facing. It sounds great for a > fellow developer reading the code, but as a user calling toFlatFormat I'd want > more information. Perhaps something along the lines of: > > /** > * Writes to a simple flat format. The details of this format have not > * been finalized and may change in the future. For now this produces a > * List that contains null, ints, strings, or nested Lists. > * > * Consider using [toStringFormat] or [toMaps] instead for most use cases. > */ > Changed. I hesitate to recommend toStringFormat in general, this one is considerably more efficient. But all of this needs to change soon anyway. Also, along the way, wondering if this might contain bools I discovered bools didn't actually work in flat format, so made code changes for that. Note that this doesn't contain nested Lists. It's just a list of primitives, so closer to something you might just write to bytes, except that it doesn't deal with the variable lengths between numeric types and of strings. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:114: var flatData = new List(); On 2012/11/20 01:20:47, John Messerly wrote: > personally I tend to use "[]" instead of "new List()" Done. I think I have a persistent glitch in my mental model that [] is const, even though I know that's not true. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:125: * Given that we have fully populated the list of [states], and more On 2012/11/20 01:20:47, John Messerly wrote: > Should we check these preconditions in code? > > Since this type+method is public, I'm wondering if a user could get messed up by > calling methods in the wrong order and violating the invariants. I haven't yet done a thorough job of figuring out what needs to be private, but this seemed like a strong candidate, and the only reason it needed to be public was because it was called from a test that was written before the rest of write() was available. So changed the test and made it private. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:163: new Reference(this, rule.number, _nextObjectNumberFor(rule))); On 2012/11/20 01:20:47, John Messerly wrote: > +2 indent Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:225: /** Return the serialized data in string format. Currently hard-coded to On 2012/11/20 01:20:47, John Messerly wrote: > newline after /** here and in toJSON Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:235: String toJSON(data) { On 2012/11/20 01:20:47, John Messerly wrote: > I'm not sure this needs a function, perhaps just inline the JSON.stringify call? Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:247: * This effectively defines a custom JSON serialization format, although On 2012/11/20 01:20:47, John Messerly wrote: > Is there a way I can use serialization with existing JSON formats, which don't > have rules/data/roots? Not yet, that's part of the generalization to support more pluggable output formats that still needs to happen, and which has several TODOs in various places in the code. For the existing formats they also won't understand the Reference objects, so it needs to be nested maps rather than a series of flat ones. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:270: roots.map((x) => _referenceFor(x)); On 2012/11/20 01:20:47, John Messerly wrote: > _rootReferences(roots) => roots.map(_referenceFor); Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:359: get rules => serialization.rules; On 2012/11/20 01:20:47, John Messerly wrote: > these could use doc comments+type annotation, if they are public facing Done. Commented and typed two, deleted one. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:383: return roots.map((x) => inflateReference(x)); On 2012/11/20 01:20:47, John Messerly wrote: > could be "return roots.map(inflateReference)" ? Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:438: => read(input, externals)[0]; On 2012/11/20 01:20:47, John Messerly wrote: > for consistency put => on previous line Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:529: if (anObject is Map && (anObject["__Ref"] == true)) { On 2012/11/20 01:20:47, John Messerly wrote: > parens not needed for the == expr (grumble, grumble, yes, but makes the code clearer) Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:544: class Sentinel { On 2012/11/20 01:20:47, John Messerly wrote: > Might want to make the type library private, so no one else can type test or > create a Sentinel. Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:564: Queue queue = new Queue(); On 2012/11/20 01:20:47, John Messerly wrote: > final? likewise for roots? roots can't be final because of the addRoots() method. But made queue and writer final. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/reader_writer.dart:634: "object" : objectNumber}; On 2012/11/20 01:20:47, John Messerly wrote: > I think we usually +2 more indent here (which we inherited from Java style > guide). alternatively, this style works too: > > toJson() => { > "__Ref" : true, > "rule" : ruleNumber, > "object" : objectNumber > }; Done. Used second style. Now if I can just have that auto-formatter... https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... File pkg/serialization/lib/src/serialization_helpers.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:7: * serialiation. Some or all of them might be removable with changes to the On 2012/11/20 01:20:47, John Messerly wrote: > perhaps reword second sentence? > > Some or all of these will be removed once the functionality is available in the > core library. Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:19: /** Concatenate two lists. Handle the case where one or both might be null. */ On 2012/11/20 01:20:47, John Messerly wrote: > this seems to me like it should exist in core libraries somewhere? if so, do you > mind filing a bug? with a bug+TODO it will be easier to track if we can remove > it later. Done. Bug existed already, but added the TODO referencing it. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:22: if (b == null) return []; else return new List.from(b); On 2012/11/20 01:20:47, John Messerly wrote: > should use curlies here: > http://www.dartlang.org/articles/style-guide/#do-use-curly-braces-for-all-flo... Rewrote it using ? : https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:56: class MapLikeIterable { On 2012/11/20 01:20:47, John Messerly wrote: > should this iterate a key-value pairs? then it could implement Collection for > real. > > in C#, Dictionary implements Enumerable<KeyValuePair<K,V>>, so all of the > list-related functionality "just works" on pairs. It's possible, but then the iteration would be different from Map. I also worried a bit about it being a performance impact, and also less convenient, because the user has to extract out the key and value themselves. I'm hopeful that some sort of mechanism for this will be in the forthcoming collections library revisions, and am bugging Florian about it this week. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:62: if (collection is Map) { On 2012/11/20 01:20:47, John Messerly wrote: > this seems to me like there should be two subtypes, one for when Collection is a > Map, and another if it is a generic iterable. Generally lots of type tests imply > polymorphism would be a cleaner solution. Yes, these are definitely a short-term hack to do the basic things I needed. Re-organizing to have separate subclasses is definitely nicer, though, and did that. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:63: collection.forEach(f); On 2012/11/20 01:20:47, John Messerly wrote: > not sure how I feel about forEach and iterator behaving differently. iterator is > iterating over values but forEach is iterating over pairs... Yes, that's unpleasant. The iterator method wasn't actually used, so deleted it. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:125: class ListLikeIterable { On 2012/11/20 01:20:47, John Messerly wrote: > I'm not seeing the point of this class. If collection is! Map, then all the > methods simply forward. If the collection is a Map, then it behaves like > MapLikeIterable, except for forEach... > > Personally, I would try to redesign these two types (ListLike and MapLike) and > see if: > * they can both be defined as "implements Collection<T>" (or List<T>) and use > generics > * they can avoid wrapping normal lists Split this out so that if you call values() on an Iterable you just get the same thing back, and simplified this code so that it's just for Maps. The things it provides are a forEach() that takes one parameter, and a map() operation. On redesigning the two, implementing either Collection or List would drag along a lot more methods that aren't used right now, which I'm reluctant to do for code that I'm hoping I'll be able to just delete soon and use library facilities that are more general-purpose. It doesn't seem like it would be easy to implement either, because the point of MapLikeIterable is to behave like a Map. Map doesn't implement Collection, and can't, because its definitions conflict, notably forEach. It might be possible for MapLikeIterable to implement Map and ListLikeIterable to implement List, although there are some questionable operations there, e.g. I don't really want to provide []=(int x) on a Map with semantics of picking an arbitrary ordering and replacing the element with that index. And if its semantics were instead to just delegate to the map's []= operation then the type signature isn't right. For the purposes of this code, a generic parameter wouldn't be much use because most of the uses would be <dynamic>. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_helpers.dart:132: collection.forEach((key, value) => f(value)); On 2012/11/20 01:20:47, John Messerly wrote: > is this just collection.values.forEach(f)? Yes, but avoids asking for values, which can be an expensive operation (i.e. copying). It's starting to sound like the intention is that keys/values can be relied on to be fast, although looking at the actual implementation some have it directly available as a field and some (e.g. _LinkedHashMapImpl) are copying. I think this is definitely something that the new libraries will fix by having lazy iterables. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... File pkg/serialization/lib/src/serialization_rule.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_rule.dart:15: int number; On 2012/11/20 01:20:47, John Messerly wrote: > Is this the index of the rule? I can't recall how this was used, but it would be > nice if this could be passed in at the appropriate places. That would avoid > duplication and potential bugs around inconsistency (if number becomes different > from the rule's index). I think that would be quite tricky. I definitely wanted the rule to know its own number so that it's efficient to find it given the rule. And it's set automatically when adding the rule, so it should be quite difficult for it to get out of sync. It might be nice to make it final, but that's tricky to reconcile with the user being able to create their own rule subclasses and add them. Decided to do the poor man's final, and made a getter/setter that throw if you try to change the number after it's been set. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_rule.dart:72: inflateEssential(state, reader); On 2012/11/20 01:20:47, John Messerly wrote: > Apologies if I mentioned this before -- I find "essential" terminology a bit > confusing. I had never heard this term before. Perhaps: "inflateConstrutorArgs" > and "inflateSetters" ? I think typical developers would be more likely to guess > the meaning with a name like that. I made up the terminology, and we can certainly change it. The problem with calling it just constructor args is that that's true most of the time, but not all, because you can tell it to treat other things as essential. There are things that get special treatment, and in some circumstances you want to treat the contents of a collection as essential. Saying that the contents of this list should be treated as constructor arguments seems equally confusing. I think most users shouldn't ever have to understand this terminology. When you're calling addRule: you specify constructorFields and fields, and possibly specialTreatmentFor. So this only would come up if someone were writing their own rule subclass or when creating a ClosureToMapRule, where the closures are called construct and setNonEssentialState. We could certainly change the second name. Or we could change the terminology in general, but I think it has to be a bit more general than constructor arguments. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_rule.dart:261: return (object is num) || (object is String) || (object is bool); On 2012/11/20 01:20:47, John Messerly wrote: > personally I wouldn't use parens here. C++ precedence rules can be confusing, > but it's generally understood that || and && are lower than comparisons, and > that tends to hold across languages Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_rule.dart:280: Type type; On 2012/11/20 01:20:47, John Messerly wrote: > final? Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/src/serialization_rule.dart:347: inflateNonEssential(object, _, Reader r) {} On 2012/11/20 01:20:47, John Messerly wrote: > seems unusual to see "_" in the middle of a method declaration. Perhaps just use > "(state, object, Reader r)" as above Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... File pkg/serialization/test/polyfill_identity_set_test.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/test/polyfill_identity_set_test.dart:13: import '../../unittest/lib/unittest.dart'; On 2012/11/20 01:20:47, John Messerly wrote: > use "package:" here if you can (maybe doesn't work yet?) It's supposed to now, so converted all of these. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... File pkg/serialization/test/serialization_test.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/test/serialization_test.dart:399: (map, object) { object On 2012/11/20 01:20:47, John Messerly wrote: > personally I'd put "object" on next line, and only indent the block by 2: > http://www.dartlang.org/articles/style-guide/#do-indent-blocks-with-two-spaces Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/test/serialization_test.dart:463: return rules.map( (x) => x.extractState(object, doNothing)); On 2012/11/20 01:20:47, John Messerly wrote: > looks like an extra space here Done.
https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... File pkg/serialization/lib/serialization.dart (right): https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:18: * ..addRuleFor(address); On 2012/11/20 02:05:30, justinfagnani wrote: > I'm confused by this line here: does it att a rule for the instance address, or > the class Address? If it's the class, consider using Type as the parameter type > and address.runtimeType in the example code for now. This is all just workaround for being able to use the type name. But right now the only way I know of to get a ClassMirror is via an instance. Even using anInstance.runtimeType doesn't work, because a mirror on the type is an instanceMirror of an instance of Type. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:21: * This creates a new serialization and adds a rule for address objects. Right On 2012/11/20 02:05:30, justinfagnani wrote: > Why is it called "serialization" and not "serializer". I would expect that a > serialization is the result of serializing some object. Because it's better to name objects for what they represent than what they do? Also, I'd think a serializer is an object that serializes, which this doesn't do - that's Writer (which I wish I had a non "er" name for). Rather than being the thing that serializes, this is the definition of a serialization scheme. So grammatically it makes sense to me as serialization. You define a serialization. In some cases you have different objects on the client side and on the server, so you need to use different serializations in each place. And so on. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:31: * ..addRuleFor(address, On 2012/11/20 02:05:30, justinfagnani wrote: > "Rule" as a name here doesn't seem ideal to me. Every other time I've > encountered this concept it's usually been named "custom serializer" or > something similar. Hmm. Rule is a lot shorter than custom serializer. These are also, or at least can be, depending on the implementation, reasonably declarative, which I thought fit more with the idea of a rule. Saying that a serialization is defined by a series of rules seems to make sense. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:36: * This rule still uses reflection to access the fields, but not to calculate On 2012/11/20 02:05:30, justinfagnani wrote: > Is there a way to clarify the phrase "not to calculate them"? maybe "not > determine which fields to serialize" Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:54: * new ClosureToMapRule(anAddress.runtimeType, On 2012/11/20 02:05:30, justinfagnani wrote: > When using many closures together I find it cleaner to just define a class > rather than to pass them all to a constructor. As a writer of rules, I'd rather > write: > > class AddressRule extends Rule<Address> { > final Type type = anAddress.type; // fix when literals are supported > Map toMap(a) => {"number" : a.number, "street" : a.street, "city" : a.city}; > Address fromMap(Map m) => new Address.create(m["number"], m["street"]); > void fillIn(Address a, Map m) => a.city = m["city"]; > } > > var serialization = new Serialization()..addRule(new AddressRule()); Yes. And the intent is that people can do that, which you'd do just by subclassing SerializationRule. So there's not much for the framework to do. But that should have an example, and some documentation, and possibly an abstract superclass designed to be subclassed for that purpose. Added a TODO (in serialization_rule.dart). https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:57: * Note that there are three different functions provided. The first one On 2012/11/20 02:05:30, justinfagnani wrote: > "first one" -> "addressToMap" Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:59: * map. The second one creates a new address using a map like the one returned On 2012/11/20 02:05:30, justinfagnani wrote: > "second one" -> "createAddress" Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:61: * created object. At the moment, however, this is more likely to cause problems On 2012/11/20 02:05:30, justinfagnani wrote: > Consider breaking this paragraph up, maybe right before "At the moment," Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:74: * It is possible to give constructor fields values that aren't field names. If On 2012/11/20 02:05:30, justinfagnani wrote: > I don't understand this paragraph. Rewrote. It's a bit of a bells and whistles feature, but something I needed for the meta-serialization. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:84: * s..addRuleFor(fooHolderInstance).specialTreatmentFor("foo", On 2012/11/20 02:05:30, justinfagnani wrote: > I feel like "specialTreatementFor" is pretty long and doesn't denote that the > treatment is for a field, which is important because the types don't indicate > that either. How about something like handleField()? I agree it's too long. But handleField() seems very vague. Maybe specialField()? customField()? I figured the parameter being called fieldName was enough indication of what was expected, but certainly don't mind including "field" in the name. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:111: * List input = serialization.readFlat(aList); On 2012/11/20 02:05:30, justinfagnani wrote: > Does this return a List of Lists? From the description of writeFlat above it > sounds like a single object can be written to a list, so multiple objects would > be written to nested lists. No, it returns a list of the roots that were written. I think the whole thing around single and multiple objects is confusing, and am leaning towards always having a single root so read/write methods just return one object. If you want multiples, give it a List as the root. Thoughts? https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:113: * There is also a convenience method for the case of reading a single object. On 2012/11/20 02:05:30, justinfagnani wrote: > Are there corresponding methods to write multiple objects? See above. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:116: * Object result = serialization.readOneFlat(aString); On 2012/11/20 02:05:30, justinfagnani wrote: > For multiple objects it would be great to have the equivalent of Java's > ObjectInputStream and ObjectOutputStream. If the objects written/read are entirely independent then it seems like it would be pretty straightforward, especially with the new streaming stuff. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:128: * When reading, some object references should not be serialized, but should be On 2012/11/20 02:05:30, justinfagnani wrote: > Why wouldn't this be done with a custom rule for that type? The rule can > serialize the external object with the info needed to resolve it on the > receiver, and the receiver and then resolve it however it needs to. This could > be done with mirrors or without depending on the situation/environment. It's not by type, but by instance. Or at least potentially by instance. This Person should be de-serialized, but that one should be looked up in the session. So you shouldn't even need a custom rule for it. I'm thinking that the appropriate generalization, at least for the easy cases, is that on write you also can provide a dictionary mapping from instances to names, and any instances on that list get mapped to the appropriate name. In cases where it should apply to a whole type a custom rule would work. This particular rule doesn't actually use mirrors, it's used in order to de-serialize references to mirrors. Which is more confusing than it needs to be, it was just done that way because that was the initial use case I had. This support definitely needs to be generalized, and there are TODOs for that in here. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:143: import 'src/mirrors_helpers.dart'; On 2012/11/20 02:05:30, justinfagnani wrote: > I think we're supposed to use package: urls even for internal imports. Maybe > double check w/ someone in SEA though. We weren't for stuff in the SDK, because the bots can't handle them. But the CL to fix that is in review, so hopefully very soon now it'll work. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:145: //import 'src/polyfill_identity_set.dart'; On 2012/11/20 02:05:30, justinfagnani wrote: > remove Done. https://chromiumcodereview.appspot.com/11293283/diff/15013/pkg/serialization/... pkg/serialization/lib/serialization.dart:221: // TODO(alanknight): Take a type rather than an instance. Issue 6282. On 2012/11/20 02:05:30, justinfagnani wrote: > Can you still take a type and require that callers use .runtimeType() to get > one? Sadly, no. Checked to see if there was a bug for that and didn't see anything, so added that to 6433.
lgtm other than one question about the status file. Will be great to get this in, and try it out and iterate :) https://chromiumcodereview.appspot.com/11293283/diff/9003/pkg/pkg.status File pkg/pkg.status (right): https://chromiumcodereview.appspot.com/11293283/diff/9003/pkg/pkg.status#newc... pkg/pkg.status:47: serialization/test/serialization_test.dart did you mean to write ": Skip" here?
Got the one LGTM, so going to commit this and incorporate any further feedback in subsequent CLs. https://chromiumcodereview.appspot.com/11293283/diff/9003/pkg/pkg.status File pkg/pkg.status (right): https://chromiumcodereview.appspot.com/11293283/diff/9003/pkg/pkg.status#newc... pkg/pkg.status:47: serialization/test/serialization_test.dart On 2012/11/21 01:47:18, John Messerly wrote: > did you mean to write ": Skip" here? Oops. Done. |
