Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support serialization of collections that are not lists #1821

Merged
merged 5 commits into from Jan 28, 2022

Conversation

sschuberth
Copy link
Contributor

So far there was an implicit assumption hard-coded that collections are
always lists. However, sets are also collections, and can be serialized
to JSON arrays just like lists.

This change allows to serialize generic collections independently of the
concrete implementation.

Fixes #1421.

@sschuberth sschuberth marked this pull request as ready for review January 9, 2022 12:55
So far there was an implicit assumption hard-coded that collections are
always lists. However, sets are also collections, and can be serialized
to JSON arrays just like lists.

This change allows to serialize generic collections independently of the
concrete implementation.

Fixes Kotlin#1421.
Now with `CollectionSerializer` inheriting from `ListLikeSerializer`, it
makes sense to rename `ListLikeSerializer` to the more generic
`CollectionLikeSerializer`.
@sschuberth
Copy link
Contributor Author

Reviews anyone? @sandwwraith maybe?

@sandwwraith
Copy link
Member

@sschuberth Sorry, I was on vacation. I'll return to your PR in a couple of days.

@sandwwraith sandwwraith self-requested a review January 17, 2022 13:30
@sandwwraith
Copy link
Member

Please also execute ./gradlew apiDump so check task would not fail

Done by running `./gradlew apiDump`.
@sschuberth
Copy link
Contributor Author

Please also execute ./gradlew apiDump so check task would not fail

I've added a new commit that updates the API dump for all introduced changes at once, I hope that's fine. Let me know if you want API dump changes squashed into the commit that do the actual changes instead.

Copy link
Member

@sandwwraith sandwwraith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, great job!

@sandwwraith sandwwraith merged commit d254e6d into Kotlin:master Jan 28, 2022
@sandwwraith
Copy link
Member

Oops. I've just realized that this PR is opened against master, while it should be merged into dev. I think I will leave it in master and cherry-pick to dev manually.

sandwwraith pushed a commit that referenced this pull request Jan 28, 2022
So far there was an implicit assumption hard-coded that collections are
always lists. However, sets are also collections, and can be serialized
to JSON arrays just like lists.

This change allows serializing generic collections independently of the
concrete implementation.

Fixes #1421.

* CollectionSerializers: Remove redundant visibility modifiers

* SealedGenericClassesTest: Remove unused variables to avoid warnings

* Rename `ListLikeSerializer` to `CollectionLikeSerializer`

Now with `CollectionSerializer` inheriting from `ListLikeSerializer`, it
makes sense to rename `ListLikeSerializer` to the more generic
`CollectionLikeSerializer`.

(cherry picked from commit d254e6d)
@sschuberth sschuberth deleted the collection-serializer branch January 28, 2022 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serialization of Set as Collection does not work
3 participants