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

gen-swagger: use GetJsonName for struct member names #681

Closed
wants to merge 1 commit into from
Closed

gen-swagger: use GetJsonName for struct member names #681

wants to merge 1 commit into from

Conversation

utrack
Copy link
Contributor

@utrack utrack commented Jun 24, 2018

This PR fixed Swagger definition.

Currently protoc-gen-swagger uses .GetName() to get a field name.
It's wrong, since proto field snake_case's generated name in json tag is snakeCase - however, generated swagger still has snake_case in the definition.
.GetJsonName() returns field name in the right case and form.

@achew22
Copy link
Collaborator

achew22 commented Jun 24, 2018

Hey @utrack, thanks for sending in a PR! This is definitely a change I would like to make. The swagger generator is objectively wrong. Unfortunately it is wrong in a way that is paired with our default marshaller. In order to make this change we would have to change the default marshaller, which is a kind of big change.

What would you think about adding this as a flag controlled argument to the swagger generator? That would let you continue forward while I try to figure out a path to removing the bad marshaller.

@utrack
Copy link
Contributor Author

utrack commented Jun 24, 2018

Oh dang, I just noticed I've already filled #375 :)

I see that the new flag should be added via common registry - it would be confusing for the flag to work w/ -gen-swagger, but not with -gen-gateway.

I'll maintain my fork w/ -gen-go-style JSON tags and wait for the bad marshaller replacement meanwhile.

What's the issue related to the marshaller replacement, if there's any? I might be able to help with that.

brocaar added a commit to brocaar/chirpstack-application-server that referenced this pull request Jul 11, 2018
This fork uses the correct JSON names as specified by the Protobuf to
JSON mapping
(https://developers.google.com/protocol-buffers/docs/proto3#json).

This is needed until
grpc-ecosystem/grpc-gateway#681 has been merged.
@johanbrandhorst
Copy link
Collaborator

This is already being considered in #546

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

Successfully merging this pull request may close these issues.

4 participants