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 protoc-gen-swagger to output gRPC method summary and descriptions as Swagger's them #667

Merged
merged 2 commits into from Jun 22, 2018

Conversation

Projects
None yet
5 participants
@co3k
Contributor

co3k commented Jun 11, 2018

No description provided.

@googlebot googlebot added the cla: yes label Jun 11, 2018

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 11, 2018

Codecov Report

Merging #667 into master will decrease coverage by 0.09%.
The diff coverage is 5.26%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #667     +/-   ##
=========================================
- Coverage   56.43%   56.34%   -0.1%     
=========================================
  Files          30       30             
  Lines        3005     3010      +5     
=========================================
  Hits         1696     1696             
- Misses       1145     1150      +5     
  Partials      164      164
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/template.go 38.27% <5.26%> (-0.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b9677c...9566623. Read the comment docs.

codecov-io commented Jun 11, 2018

Codecov Report

Merging #667 into master will decrease coverage by 0.09%.
The diff coverage is 5.26%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #667     +/-   ##
=========================================
- Coverage   56.43%   56.34%   -0.1%     
=========================================
  Files          30       30             
  Lines        3005     3010      +5     
=========================================
  Hits         1696     1696             
- Misses       1145     1150      +5     
  Partials      164      164
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/template.go 38.27% <5.26%> (-0.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b9677c...9566623. Read the comment docs.

@achew22

This comment has been minimized.

Show comment
Hide comment
@achew22

achew22 Jun 12, 2018

Collaborator

@co3k, thanks so much for your pull request. Looking through, I'm a bit puzzled by what it should do. It looks like the changes to the golden .go files all come from changes upstream. If this code path isn't exercised by our examples, do you think you could add an example that does exercise it so I can see the new functionality?

Collaborator

achew22 commented Jun 12, 2018

@co3k, thanks so much for your pull request. Looking through, I'm a bit puzzled by what it should do. It looks like the changes to the golden .go files all come from changes upstream. If this code path isn't exercised by our examples, do you think you could add an example that does exercise it so I can see the new functionality?

@co3k

This comment has been minimized.

Show comment
Hide comment
@co3k

co3k Jun 12, 2018

Contributor

@achew22

It looks like the changes to the golden .go files all come from changes upstream

I added these changes to pass build on Travis CI. In the other pull request, I've encountered build error that is caused by the upstream changes. https://travis-ci.org/grpc-ecosystem/grpc-gateway/builds/390065893

do you think you could add an example that does exercise it so I can see the new functionality?

You're right. I've added the ce1532e commit to show it.

Contributor

co3k commented Jun 12, 2018

@achew22

It looks like the changes to the golden .go files all come from changes upstream

I added these changes to pass build on Travis CI. In the other pull request, I've encountered build error that is caused by the upstream changes. https://travis-ci.org/grpc-ecosystem/grpc-gateway/builds/390065893

do you think you could add an example that does exercise it so I can see the new functionality?

You're right. I've added the ce1532e commit to show it.

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 20, 2018

Collaborator

Hi! Can you please rebase this on top of master? There was recently a change to the swagger generator. You can run something like:

make realclean && make examples SWAGGER_CODEGEN="java -jar $HOME/local/swagger-codegen-cli.jar"

from the root of the repo to regenerate all examples. This will require swagger-codegen (and Java) installed.

Collaborator

johanbrandhorst commented Jun 20, 2018

Hi! Can you please rebase this on top of master? There was recently a change to the swagger generator. You can run something like:

make realclean && make examples SWAGGER_CODEGEN="java -jar $HOME/local/swagger-codegen-cli.jar"

from the root of the repo to regenerate all examples. This will require swagger-codegen (and Java) installed.

@co3k

This comment has been minimized.

Show comment
Hide comment
@co3k

co3k Jun 20, 2018

Contributor

@johanbrandhorst I rebased my changes and tests on Travis CI are passed.

Contributor

co3k commented Jun 20, 2018

@johanbrandhorst I rebased my changes and tests on Travis CI are passed.

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 20, 2018

Collaborator

Hi, could you squash the commits as well please? Sorry I didn't specify this in the last message.

Collaborator

johanbrandhorst commented Jun 20, 2018

Hi, could you squash the commits as well please? Sorry I didn't specify this in the last message.

@co3k

This comment has been minimized.

Show comment
Hide comment
@co3k

co3k Jun 21, 2018

Contributor

@johanbrandhorst OK, I've just squashed them.

Contributor

co3k commented Jun 21, 2018

@johanbrandhorst OK, I've just squashed them.

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 21, 2018

Collaborator

Thanks, I will leave it for @achew22 to approve and merge, but I'm a little confused by the PR description. Looking at the code this change looks like it parses the method comment and adds it to "Summary" and "Description" in accordance with other such comment parsing, but the PR title, description and commit message is a little confusing. Could you reword it for clarity please?

Collaborator

johanbrandhorst commented Jun 21, 2018

Thanks, I will leave it for @achew22 to approve and merge, but I'm a little confused by the PR description. Looking at the code this change looks like it parses the method comment and adds it to "Summary" and "Description" in accordance with other such comment parsing, but the PR title, description and commit message is a little confusing. Could you reword it for clarity please?

@achew22 achew22 merged commit 11bef10 into grpc-ecosystem:master Jun 22, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@achew22

This comment has been minimized.

Show comment
Hide comment
@achew22

achew22 Jun 22, 2018

Collaborator

@johanbrandhorst another neat thing about the squash button on github is that you enter the description of the new squashed commit which makes it perfect for these situations. 2 commits squashed down into 1 and then the description hopefully clarified.

@co3k Thanks so much for your contribution! This will help in producing great documentation for APIs.

Collaborator

achew22 commented Jun 22, 2018

@johanbrandhorst another neat thing about the squash button on github is that you enter the description of the new squashed commit which makes it perfect for these situations. 2 commits squashed down into 1 and then the description hopefully clarified.

@co3k Thanks so much for your contribution! This will help in producing great documentation for APIs.

@co3k co3k deleted the co3k:fix-not-to-output-method-comments-as-swagger-description branch Jun 28, 2018

@co3k co3k changed the title from Fix protoc-gen-swagger not to output gRPC method summary and descriptions as Swagger's them to Fix protoc-gen-swagger to output gRPC method summary and descriptions as Swagger's them Jun 28, 2018

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