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

fix issue #371 #372

Closed
wants to merge 1 commit into from
Closed

Conversation

cloudsinmycoffee
Copy link

@cloudsinmycoffee cloudsinmycoffee commented Nov 16, 2022

Issue #371:

Description of changes:
generator.go in func doJSONRequest was changed from

	t.P(`  marshaler := &`, t.pkgs["protojson"], `.MarshalOptions{UseProtoNames: true}`)

to

	t.P(`  marshaler := &`, t.pkgs["protojson"], `.MarshalOptions{UseProtoNames: !s.jsonCamelCase, EmitUnpopulated: !s.jsonSkipDefaults}`)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -819,7 +819,7 @@ func (t *twirp) generateUtils() {

t.P(`// doJSONRequest makes a JSON request to the remote Twirp service.`)
t.P(`func doJSONRequest(ctx `, t.pkgs["context"], `.Context, client HTTPClient, hooks *`, t.pkgs["twirp"], `.ClientHooks, url string, in, out `, t.pkgs["proto"], `.Message) (_ `, t.pkgs["context"], `.Context, err error) {`)
t.P(` marshaler := &`, t.pkgs["protojson"], `.MarshalOptions{UseProtoNames: true}`)
t.P(` marshaler := &`, t.pkgs["protojson"], `.MarshalOptions{UseProtoNames: !s.jsonCamelCase, EmitUnpopulated: !s.jsonSkipDefaults}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this would compile. The s receiver (server) is not accessible on this function. This function is used by the client to send requests, and the client constructor currently doesn't have options like the ones for the server WithServerJSONSkipDefaults and WithServerJSONCamelCaseNames.

To verify this, check the instructions on CONTRIBUTING.md on how to re-generate the code and run tests with the newly generated code.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the info. I will check on how to fix this issue properly and improve my pull request according to the instructions.

@github-actions
Copy link

This PR is stale because it has been open for 60 days with no activity. Remove stale label / comment or this will be closed in 5 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants