Add support for generic classes #169
Conversation
Why couldn't they be separate PRs? |
|
Also, why does a commit which adds over 400 lines have a 5 word commit message? |
| @JsonSerializable() | ||
| class GenericClass<T extends num, S> extends Object | ||
| with _$GenericClassSerializerMixin<T, S> { | ||
| @JsonKey(fromJson: _dataFromJson, toJson: _dataToJson) |
There was a problem hiding this comment.
are _dataFromJson and _dataToJson required for this to work?
There was a problem hiding this comment.
Yup. Will document as such.
| String _wrapperClassName([bool withConstraints]) => | ||
| '${_prefix}JsonMapWrapper${_genericClassArguments(withConstraints)}'; | ||
|
|
||
| String _genericClassArguments(bool withConstraints) { |
| @@ -39,8 +39,20 @@ class _GeneratorHelper { | |||
| final StringBuffer _buffer = new StringBuffer(); | |||
|
|
|||
| String get _prefix => '_\$${_element.name}'; | |||
| String get _mixClassName => '${_prefix}SerializerMixin'; | |||
| String get _helpClassName => '${_prefix}JsonMapWrapper'; | |||
| String _mixinClassName(bool withConstraints) => | |||
There was a problem hiding this comment.
consider making this 2 separate methods (or getters) rather than using a flag argument. The names will be more clear about what is happening than a cryptic "true" or "false" for which you need to see the argument name to understand.
| final $_mixClassName _v; | ||
| $_helpClassName(this._v); | ||
| _buffer | ||
| .writeln('''class ${_wrapperClassName(true)} extends \$JsonMapWrapper { |
There was a problem hiding this comment.
with writeln you're going to get an extra newline and this already ends in a newline. Do we need to?
There was a problem hiding this comment.
Honestly, dartfmt handles most of this. Not going to modify this in this CL
json_serializable/lib/src/utils.dart
Outdated
| @@ -175,7 +175,19 @@ int _sortByLocation(FieldElement a, FieldElement b) { | |||
|
|
|||
| final _dartCoreObjectChecker = const TypeChecker.fromRuntime(Object); | |||
|
|
|||
| /// If a parameter is required to invoke the constructor, | |||
| String genericClassArguments(ClassElement element, bool withConstraints) { | |||
There was a problem hiding this comment.
we seem to be relying on withConstraints being non-null here because of another method with the same name handling nullability. Maybe handle them both here and skip the second identically named wrapper method?
There was a problem hiding this comment.
Ack. Going to leave both. Having the helper in generator_helper is nice.
Did forward the null check to the inner method
json_serializable/lib/src/utils.dart
Outdated
| @@ -175,7 +175,19 @@ int _sortByLocation(FieldElement a, FieldElement b) { | |||
|
|
|||
| final _dartCoreObjectChecker = const TypeChecker.fromRuntime(Object); | |||
|
|
|||
| /// If a parameter is required to invoke the constructor, | |||
| String genericClassArguments(ClassElement element, bool withConstraints) { | |||
json_serializable/lib/src/utils.dart
Outdated
| } | ||
| var values = element.typeParameters.map((t) { | ||
| if (withConstraints) { | ||
| return t.toString(); |
There was a problem hiding this comment.
consider ternary conditional when what you care about is the value rather than side effects
json_serializable/lib/src/utils.dart
Outdated
| @@ -243,7 +255,10 @@ CtorData writeConstructorInvocation( | |||
| writeableFields.toSet().difference(usedCtorParamsAndFields); | |||
|
|
|||
| var buffer = new StringBuffer(); | |||
| buffer.write('new $className('); | |||
| // | |||
| // Generate the static factory method | |||
There was a problem hiding this comment.
should this be a separate method named generateStaticFactory?
There was a problem hiding this comment.
This is an old comment. This does not create a factory. It creates an invocation. Will remove.
| @@ -0,0 +1,41 @@ | |||
| // Copyright (c) 2017, the Dart project authors. Please see the AUTHORS file | |||
...and some name cleanup.
I will land these are separate commits, @natebosch 😄