Skip to content
This repository was archived by the owner on May 25, 2022. It is now read-only.

Conversation

davidmorgan
Copy link
Contributor

This change is Reviewable

@BlackHC
Copy link
Contributor

BlackHC commented Mar 17, 2016

Review status: 0 of 20 files reviewed at latest revision, 4 unresolved discussions.


built_json/lib/built_json.dart, line 155 [r1] (raw file):
Is specifiedType needed for primitive types?

The doc comments need to be updated for both classes btw. This one is for primitive types (as defined in Dart) and the one below for composite types or whatever the right term is ^^


built_json/lib/src/built_json_serializers.dart, line 128 [r1] (raw file):
Is this necessary? Sorry for nitpicking. (Could we also just use marker interfaces?)


built_json/lib/src/built_map_serializer.dart, line 60 [r1] (raw file):
Bit magic? Why not % 2?


built_json/lib/src/built_set_serializer.dart, line 40 [r1] (raw file):
Why? (readability)


Comments from the review on Reviewable.io

@davidmorgan
Copy link
Contributor Author

Review status: 0 of 20 files reviewed at latest revision, 4 unresolved discussions.


built_json/lib/built_json.dart, line 155 [r1] (raw file):
It's not used right now, but it's conceivable, particularly if people want to plug in other serialization systems. e.g. if you wanted something that serializes everything to a single String, it would be a PrimitiveSerializer but would care about generics.

Re: the doc comments, yeah, that was a big ugly. There is really no inheritance for serialize/deserialize, they're disjoint. So I've moved them onto the subclasses and added separate docs. The downside is it means I have to add another branch to the "if" clause in built_json_serializers: if (is StructuredSerializer) else if (is PrimitiveSerializer) else {}. This is clearer though. Done.


built_json/lib/src/built_json_serializers.dart, line 128 [r1] (raw file):
Yeah, I think we need both types, now that I moved the methods to where they should really be the top level "serializer" class can't serialize or deserialize anything. But, we still need that interface so we can find the right serializer. PTAL.


built_json/lib/src/built_map_serializer.dart, line 60 [r1] (raw file):
Dunno, for me it reads the same :) ... changed.


built_json/lib/src/built_set_serializer.dart, line 40 [r1] (raw file):
Underspecified means there's no information about the types it contains, so we have to create a Set.


Comments from the review on Reviewable.io

@BlackHC
Copy link
Contributor

BlackHC commented Mar 17, 2016

:lgtm:


Review status: 0 of 20 files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

davidmorgan added a commit that referenced this pull request Mar 18, 2016
Add PrimitiveSerializer and StructuredSerializer interfaces.
@davidmorgan davidmorgan merged commit c314cf1 into google:master Mar 18, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants