feat: implement first version of sealed class serialization #1483
Conversation
There was a problem hiding this comment.
I'm impressed, yo. A lot of folks have asked for this.
I think I like your approach. You're absolutely right, the devil is in the details here...lots of overlapping issues.
I'd avoid the generic factories for now. It'll likely be a HUGE mess that won't affect most folks.
If you look at many of my previous PRs, there is usually more code in tests than there is in the actual implementation.
If you're game to run with what you have, I'm happy to support you. Very cool!
| /// If [functionBodyParts] has only one element, expression body is used. | ||
| /// | ||
| /// ```dart | ||
| /// ''' |
There was a problem hiding this comment.
shouldn't have ''' here. Just the dart code should be fine.
|
Superb! I'll continue working on this over the weekend. Haven't checked the testing practices of this project at all yet so might need some guidance on those, but let's see |
|
I highly recommend looking through other PRs that change something substantial. You'll get inspiration! |
| if (element.isSealed) { | ||
| sealedSubClasses(element).forEach((sub) { | ||
| final annotationConfig = jsonSerializableConfig(sub, _generator); | ||
|
|
||
| if (annotationConfig == null) { | ||
| throw InvalidGenerationSourceError( | ||
| 'The class `${element.displayName}` is sealed but its ' | ||
| 'subclass `${sub.displayName}` is not annotated with ' | ||
| '`JsonSerializable`.', | ||
| todo: 'Add `@JsonSerializable` annotation to ${sub.displayName}.', | ||
| element: sub, | ||
| ); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
All of the other checks we can do when generating subclass but not this one as the generator will never get there for the subclass if its not annotated. IMO would be cleaner if all the checks would be done with lookups from subclass to superclass (would not need to use the sealedSubClasses method here yet).
Should sealedSubClasses be passed to decode/encode helpers if its already evaluated here? Now its called again in the helpers...
| 'SubWithConflictingDiscriminatorWithDefaultNameExt', | ||
| 'SubWithConflictingDiscriminatorWithDefaultNameImpl', |
There was a problem hiding this comment.
There is some overlap in these as they test both the use of extends and implements keywords. The analyzer treats them differently within ClassElement (one comes from .interfaces and one from .supertype) so there is a risk that some refactor would break one but not other
There was a problem hiding this comment.
glad to have both! good thinking!
|
@kevmoo Did I miss any type of tests that would need to be added? Or combos of unsupported configurations? |
Whats included
This PR is a proposal related to #1342. Since the issue asked for a PR only to evaluate if adding this functionality is worth the added complexity, this PR DOES NOT (yet) include:
I'd be happy to contribute towards those too if the maintainers give green light.
Things to discuss / consider
genericArgumentFactoriesbe supported and how? Or should it warn / throw as invalid config combo?UnionKeyannotation similar toJsonKeyallowing naming change per sealed implementation?toJsonmethod where class fields are added to the output with spread operator