feat: implement first version of sealed class serialization#1483
feat: implement first version of sealed class serialization#1483O-Hannonen wants to merge 4 commits intogoogle:masterfrom
Conversation
kevmoo
left a comment
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!
|
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...
|
@kevmoo Did I miss any type of tests that would need to be added? Or combos of unsupported configurations? |
|
you'll need to rebase here. I'd wait until my Dart 3.8 change lands, though. |
|
rebase, please! |
167aad6 to
55a821b
Compare
|
Sorry for the delay, rebase is now done! @kevmoo |
|
willing to do a rebase here? |
21972ed to
b96ed81
Compare
|
@kevmoo sorry for the delay due to holidays, rebase is now done! |
00e16ce to
90bbeab
Compare
|
Any update? |
5e93427 to
d449416
Compare
kevmoo
left a comment
There was a problem hiding this comment.
The amount of code churn here is HUGE!
(1) Did you do other, unrelated refactoring? Please minimize this so it's easier to review
(2) Could you write up a 1-page review of what you're proposing
- What does this enable
- What does a user of json_serializable have to do to make this work
- What is changed in how json_serial is implemented to make it happen
I'm not against these changes, but I need some front-loading!
|
The large diff is primarily due to extensive integration tests and examples for the new sealed class support. I did refactor Sealed Class Support OverviewWhat it enables: User Requirements:
Implementation Changes:
|
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