-
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
fix rpc comment generation against allow_merge #1445
fix rpc comment generation against allow_merge #1445
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Codecov Report
@@ Coverage Diff @@
## master #1445 +/- ##
==========================================
+ Coverage 54.27% 54.33% +0.05%
==========================================
Files 42 42
Lines 4394 4399 +5
==========================================
+ Hits 2385 2390 +5
Misses 1752 1752
Partials 257 257
Continue to review full report at Codecov.
|
@googlebot I signed it! |
1328637
to
4568865
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
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.
Thanks for your contribution! Could you may add an example that shows how this fixes the generated comments? Could add two simple files to examples/internal/proto/examplepb
and merge them with a command in the Makefile?
In this PR comment? OK, hold on, please.
You mean adding, say, SWAGGER_EXAMPLES_MERGE=examples/internal/proto/examplepb/a.proto \
examples/internal/proto/examplepb/b.prot
$(EXAMPLE_SWAGGERSRCS_MERGE): $(SWAGGER_PLUGIN) $(SWAGGER_EXAMPLES_MERGE)
protoc -I $(PROTOC_INC_PATH) -I. -I$(GOOGLEAPIS_DIR) --plugin=$(SWAGGER_PLUGIN) --swagger_out=logtostderr=true,allow_merge=true:. $(SWAGGER_EXAMPLES_MERGE) Or is it suffice simply adding |
Hmm... the patch works for my *.proto files, but it looks it doesn't help some other (rather simple) cases. |
Lets add a new example as you suggest. Maybe name them something like |
Ok, so I figured out what going wrong. The following might solve the other bug but I'm not sure this is no side effect. diff --git a/protoc-gen-swagger/genswagger/template.go b/protoc-gen-swagger/genswagger/template.go
index da90927..27a8e76 100644
--- a/protoc-gen-swagger/genswagger/template.go
+++ b/protoc-gen-swagger/genswagger/template.go
@@ -1659,7 +1659,7 @@ func isProtoPathMatches(paths []int32, outerPaths []int32, typeName string, type
typeNameDescriptor = reflect.TypeOf((*pbdescriptor.DescriptorProto)(nil))
}
- if paths[0] != protoPathIndex(typeNameDescriptor, typeName) || paths[1] != typeIndex {
+ if paths[0] != protoPathIndex(typeNameDescriptor, typeName) {
return false
}
paths = paths[2:] The problem is occurred because What do you think of the snippet above? @johanbrandhorst |
OK, the patch above doesn't work properly if you define two ore more services in one proto file. |
086b2da
to
783c28f
Compare
783c28f
to
83bef3a
Compare
Added in PR comment.
Added. |
Thanks for your contribution! Could you cherry pick this against v2? |
Alright, I'll push another one. |
References to other Issues or PRs
Fixes #664
Have you read the Contributing Guidelines?
Yes
Brief description of what is fixed or changed
Currently
protoc-gen-swagger
does not make rpc comments properly when you giveallow_merge=yes
option.This PR fixes the problem.
For example, here is a diff between ones generated by current
master
and this PR: