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

[feat] Added Kotlinx Serialization support #656

Merged

Conversation

ctasada
Copy link
Collaborator

@ctasada ctasada commented Mar 16, 2024

Added basic support for KotlinX Serialization classes.

This new module serializes Kotlin @Serializable classes respecting the @SerialName annotation and types.

Solves #589

Copy link

netlify bot commented Mar 16, 2024

Deploy Preview for springwolf-ui ready!

Name Link
🔨 Latest commit 008943c
🔍 Latest deploy log https://app.netlify.com/sites/springwolf-ui/deploys/65feaaeb94dcc3000839d10a
😎 Deploy Preview https://deploy-preview-656--springwolf-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@timonback timonback left a comment

Choose a reason for hiding this comment

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

I like this implementation as it is a lot more compact and an add-on module seems appropriate.

Can you add a (small) test case for useFqn=true (the name field is sufficient)?

@ctasada ctasada force-pushed the ctasada/kotlin-serialization-add-on branch from 29ea6f3 to 642ddc1 Compare March 21, 2024 13:50
@ctasada
Copy link
Collaborator Author

ctasada commented Mar 21, 2024

@timonback done.

I will keep iterating over next PRs, but I think it's good enough to go, at least as a beta

@sam0r040
Copy link
Collaborator

Hi @ctasada,

we had a look at the changes and like it a lot more than the one that you have shown us before. We like to see:

  • More simplified code by using more Kotlin features unless we missed something
  • More fine grained and isolated test cases that make understanding and debugging easier

We created ctasada#3 as as suggestion for both and postpone the release till Wednesday (27.3.) to give you some time to review the suggestion. Tests can be refactored later, but we would like to make the production code simpler.

From our side it would be sufficient to just merge the PR ctasada#3 and then we can merge this PR without the need for further changes for the march release.

ctasada and others added 5 commits March 23, 2024 11:11
Added basic support for KotlinX Serialization classes.

This new module serializes Kotlin `@Serializable` classes respecting the `@SerialName` annotation and types.
…alizationModelConverter and add suggestion for more fine grained tests

Co-authored-by: Timon Back <timonback@users.noreply.github.com>
@ctasada ctasada force-pushed the ctasada/kotlin-serialization-add-on branch from 1866a0a to 008943c Compare March 23, 2024 10:11
@ctasada
Copy link
Collaborator Author

ctasada commented Mar 23, 2024

@sam0r040 @timonback I merged the suggested improvements, fixed the byte bug and refactor the tests a bit.

Hopefully we are good to go

Copy link
Member

@timonback timonback left a comment

Choose a reason for hiding this comment

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

Looks great @ctasada and looking forward to the next improvement to move it beyond the beta status.

@timonback timonback merged commit 41eebf3 into springwolf:master Mar 23, 2024
20 checks passed
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.

3 participants