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

[#477] add WithProtoJSONOptions() expose protojson marshalling options #500

Closed
wants to merge 1 commit into from

Conversation

natemurthy
Copy link

@natemurthy natemurthy commented Apr 23, 2023

r? @akshayjshah

I'd like to merge this fork from @greg-montoux / @xxgreg into the main repository. I'm experiencing the same challenges customizing my protojson marshalling/unmarshalling options as proposed in: Issue https://github.com/bufbuild/connect-go/issues/477

In my specific case, I have a large body of legacy JSON APIs maintained with lower_snake_case and the default camelCase marshaling is throwing some consistency curveballs my way.

I think this option method should be available to everyone as soon as convenience allows. My alternative is to maintain a fork this repository.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@xxgreg
Copy link

xxgreg commented Apr 23, 2023

(Replying from my other GH account)

Another API worth considering is to export a constructor for protoJSONCodec and allow setting the options there.

This would look something like:

mopts := protojson.MarshalOptions{
...
}
uopts := protojson.UnMarshalOptions{
...
}
cn.NewClient(..., cn.WithCodec(cn.NewProtoJSONCodec(mopts, upots))

It looks like the workaround in the meantime is to fork the protoJSONCodec struct, which isn't too bad either.

@akshayjshah akshayjshah self-requested a review May 1, 2023 07:21
@akshayjshah
Copy link
Member

akshayjshah commented May 1, 2023

In general, I'm reluctant to expose APIs from connect-go that don't require access to significant chunks of the package internals. Over time, this policy keeps the surface area of connect-go as small and approachable as possible.

The protojson codec is so small that I don't think the proposed API expansion carries its weight. It's relatively easy, and just as functional, to provide this configurability from a separate package that replicates the JSON marshaling logic. I've done just that, so you can use go.akshayshah.org/connectproto today. (Please do keep in mind that this is a small personal repo rather than an official Buf project!)

We're sorting out the best way to make third-party packages like these more discoverable, but hopefully this unblocks you in the short term. I'll leave this PR open for a bit in case there's more discussion, but I don't intend to merge it.

Copy link
Member

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

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

See above.

@natemurthy
Copy link
Author

natemurthy commented Jun 1, 2023

I ended up going with @akshayjshah's suggestion, thanks everyone!

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.

5 participants