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

Ways to ignore empty collections while encoding json #605

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

987Nabil
Copy link
Contributor

@987Nabil 987Nabil commented Sep 6, 2023

This pr introduces a config for the JsonCodec, to set behaviour on the codec creation level instead of classes via annotations. We can use this to configure default behaviour in zio-http for example via user config.
Or could make it the default of zio-http to ignore empty collection fields during encoding, without messing with the defaults of zio-schema.

@987Nabil 987Nabil requested a review from a team as a code owner September 6, 2023 04:18
@987Nabil 987Nabil force-pushed the optional-field-no-json-encoding branch from f4e6de1 to 4b8457e Compare September 6, 2023 04:28
vigoo
vigoo previously approved these changes Sep 6, 2023
@jdegoes
Copy link
Member

jdegoes commented Sep 6, 2023

@987Nabil Overall looks good. I am not a fan of the implicit config pattern. Do we have to make that config implicit?

@987Nabil
Copy link
Contributor Author

987Nabil commented Sep 6, 2023

@jdegoes I guess we can have overloaded methods with a config parameter. So we don't break compatibility.
I'll try it out.

@@ -58,7 +65,7 @@ object JsonCodec {
)
}

implicit def schemaBasedBinaryCodec[A](implicit schema: Schema[A]): BinaryCodec[A] =
implicit def schemaBasedBinaryCodec[A](implicit schema: Schema[A], cfg: Config): BinaryCodec[A] =
Copy link
Contributor

Choose a reason for hiding this comment

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

Now Config.default is no longer implicit, but here it is an implicit parameter which makes this a bit inconvenient. I suggest to keep an implicit def schemaBasedBinaryCodec with only schema parameter, always using Config.default, so users who want to customize it can use the explicit version.

Copy link
Contributor Author

@987Nabil 987Nabil Sep 7, 2023

Choose a reason for hiding this comment

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

Ahh sorry, I missed that part. I did it for the other methods. How you wrote it, was how I planned it.

@987Nabil 987Nabil force-pushed the optional-field-no-json-encoding branch from 67936bb to 6135e77 Compare September 7, 2023 10:05
@987Nabil 987Nabil force-pushed the optional-field-no-json-encoding branch from 6135e77 to c97c3dd Compare September 7, 2023 11:25
vigoo
vigoo previously approved these changes Sep 7, 2023
@987Nabil 987Nabil force-pushed the optional-field-no-json-encoding branch from 347fc58 to e92fac1 Compare September 7, 2023 13:12
@vigoo vigoo merged commit aa0770e into zio:main Sep 7, 2023
12 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