-
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
protoc-gen-swagger: add the package name to the tags field of each endpoint if the package name exists in the proto file #860
protoc-gen-swagger: add the package name to the tags field of each endpoint if the package name exists in the proto file #860
Conversation
Hi again @zwcn. It looks like you need to regenerate the files after adding this fix. Please see the contribution guide for more information. |
Looks like we need to update some of the tests after this change:
https://github.com/grpc-ecosystem/grpc-gateway/blob/master/examples/browser/echo_service.spec.js |
@johanbrandhorst , yes. As shown at https://github.com/grpc-ecosystem/grpc-gateway/pull/860/files#diff-eab1d67e52af78e98dd23e2b08fcc22aR25 , How do you feel about these long-name changes? |
Haha yeah I noticed. I suppose it's not wrong but they're not pretty names. I wonder if we can remove the tag from the generated names with a generator option? Please take a look. |
Hi @johanbrandhorst , Yes, the names aren't wrong. As shown at https://github.com/swagger-api/swagger-codegen/wiki/FAQ#how-do-tags-affect-the-generated-code, the i've browsed the One possible solution would be to add a new boolean flag called How do you think? |
I think we're gonna have to add a flag as you say as this could be considered backwards compatibility breaking as well. Could you update the PR with a flag please? See if you could add some tests or a new file generated with the new flag as well please. |
@johanbrandhorst : Sounds like a plan. Will do. |
Codecov Report
@@ Coverage Diff @@
## master #860 +/- ##
==========================================
- Coverage 53.08% 53.01% -0.08%
==========================================
Files 39 39
Lines 3888 3901 +13
==========================================
+ Hits 2064 2068 +4
- Misses 1630 1637 +7
- Partials 194 196 +2
Continue to review full report at Codecov.
|
Hi @johanbrandhorst , I've added a new boolean flag |
Thanks a lot for your contribution! |
…dpoint if the package name exists in the proto file (grpc-ecosystem#860) * add the package name to the tags field of each endpoint * prepend the package name only if it is defined in the proto file * fix the generate stage of the CI by checking in generated files * add a boolean flag allow_package_name_in_tags and unit tests * update bazel files * rename the flag to include_package_in_tags
Currently, given a path and a http method, there is no way to identify which package it is generated from. For example, since the following swagger file generated by
protoc-gen-swagger
contains only the service namedataService
and the rpc nameGetData
, we can't derive the package name in the proto file that definesdataService
andGetData
.This pull request prepends the package name to the service name in the tags field. For example, if the proto file includes a line
package data;
, the tags field will be["data.dataService"]
. Doing this allows us to trace back the origin of theGET /api/v1/data
http request.