-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
write query parameters to swagger definition #199
Conversation
Rafael, Thanks so much for sending in a patch! It looks like the build monster is unhappy because your change modifies the output for the example. grpc-gateway has the examples folder checked in and verifies that there is no difference between the checked in version and what comes out during CI. It looks like your change also needs to be rebased onto the latest master. Thanks again! |
@achew22 regenerated the examples and rebased. Tests are passing now. |
@@ -13,6 +13,49 @@ import ( | |||
"github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway/descriptor" | |||
) | |||
|
|||
// messageToQueryParameters converts a message to a list of swagger query parameters. | |||
func messageToQueryParameters(message *descriptor.Message, reg *descriptor.Registry, pathParams []descriptor.Parameter) ([]swaggerParameterObject, error) { | |||
var parameters []swaggerParameterObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was even better if you make variable names a bit shorter.
https://github.com/golang/go/wiki/CodeReviewComments#variable-names
@rgarcia can you rebase this again? |
Hey, sorry. Probably won't be able to get back to this until early next week. |
What are the chances of this getting merged soon? This is a feature that we would definitely like to have. |
@rgarcia are you still interested in getting this merged? |
@achew22 sorry, our team took a different direction, so I don't think I can do the follow-up to get this merged. |
@rgarcia, would you object to me taking over this PR and applying the changes? Note to people following along, if I haven't pushed a PR by Friday assume that I've fallen in a black hole and you can take a whack at it if you want |
@achew22 go for it! |
What is missing for this to be merged? Just to answer the question on deeply nested? |
Waiting this PR to be merged too! This feature is highly wanted! |
Is there any chance to merge this pull request? |
closed by #297 |
addresses #159