Support generating a custom map and using non-copying wrappers for serialization #48
Conversation
|
|
||
| // TODO(kevmoo): Add documentation | ||
| abstract class JsonMapWrapper extends MapBase<String, dynamic> { | ||
| /// This operation is not supported by an unmodifiable map. |
There was a problem hiding this comment.
Hmmm, is there a mixin for this? Otherwise this has to keep up to date with any new apis that the sdk adds....
Also, I think this should throw on putIfAbsent and addAll as well.
There was a problem hiding this comment.
Dude! WIP!
MapBase handles everything but what's overloaded here – it's nice.
There was a problem hiding this comment.
Ah ok looking at the docs for MapBase this should be fine.
| @@ -36,6 +36,8 @@ class JsonSerializableGenerator | |||
|
|
|||
| final List<TypeHelper> _typeHelpers; | |||
|
|
|||
| final bool _doNewThing = true; | |||
There was a problem hiding this comment.
Might want to be a bit more descriptive here :)
| buffer.writeln('}'); | ||
|
|
||
| if (_doNewThing) { | ||
| buffer.writeln(); |
There was a problem hiding this comment.
nit: it is probably worth breaking this out into its own function
|
|
||
| if (_doNewThing) { | ||
| buffer.writeln(); | ||
| // |
|
|
||
| if (fieldsList.every((e) => _includeIfNull(e, classIncludeIfNull))) { | ||
| // write simple `toJson` method that includes all keys... | ||
| //_writeToJsonSimple(buffer, fields); |
| } else { | ||
| // At least one field should be excluded if null | ||
| //_writeToJsonWithNullChecks(buffer, fields, includeIfNull); | ||
| buffer.writeln('@override\nIterable<String> get keys sync* {'); |
There was a problem hiding this comment.
Is it worth having the diverging code paths here? I would avoid it unless you have a compelling reason (performance?) to do this optimization.
There was a problem hiding this comment.
sync* is pretty expensive. Going to benchmark to find out.
There was a problem hiding this comment.
...in principle I agree
There was a problem hiding this comment.
You could also not use sync* and just build up a list or w/e, if you are concerned about the performance
| var jsonKeys = fields.values.map(_safeNameAccess).join(', '); | ||
|
|
||
| // TODO(kevmoo): maybe put this in a static field instead? | ||
| // const lists have unfortunate overhead |
There was a problem hiding this comment.
A static final field would be lazy initialized right? That might help startup performance.
|
But then I'm allocating a List on every call.
Again – will benchmark. :-)
…On Tue, Oct 3, 2017 at 12:54 PM, Jacob MacDonald ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In json_serializable/lib/src/json_serializable_generator.dart
<#48 (comment)>
:
> +
+ if (fieldsList.every((e) => _includeIfNull(e, classIncludeIfNull))) {
+ // write simple `toJson` method that includes all keys...
+ //_writeToJsonSimple(buffer, fields);
+
+ var jsonKeys = fields.values.map(_safeNameAccess).join(', ');
+
+ // TODO(kevmoo): maybe put this in a static field instead?
+ // const lists have unfortunate overhead
+ buffer.writeln(''' @OverRide
+ Iterable<String> get keys => const [${jsonKeys}];
+''');
+ } else {
+ // At least one field should be excluded if null
+ //_writeToJsonWithNullChecks(buffer, fields, includeIfNull);
+ ***@***.***\nIterable<String> get keys sync* {');
You could also not use sync* and just build up a list or w/e, if you are
concerned about the performance
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#48 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABCipl_HNEEJFU4YH48TBQFTivwRspVks5sopENgaJpZM4PsZW3>
.
|
kevmoo
force-pushed
the
custom_map
branch
5 times, most recently
from
99ac290 to
2188446
Compare
kevmoo
changed the title
|
@jakemac53 – this is ready for review. |
| @@ -6,102 +6,84 @@ import 'package:test/test.dart'; | |||
|
|
|||
| import 'package:json_serializable/src/utils.dart'; | |||
|
|
|||
| import 'test_files/kitchen_sink.dart'; | |||
| import 'test_files/kitchen_sink.dart' as nullable; | |||
There was a problem hiding this comment.
nit, this prefix is a bit inconsistent (not abbreviated). Maybe just opt for no prefix? Or come up with a prefix and add it to wrapped as well.
No description provided.