-
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
Add missing documentation for openapiv2 proto definition #1375
Conversation
@johanbrandhorst - this PR adds some missing documentation for open API fields. I've taken the OpenAPI spec definitions for them. |
Guess that I will need to regenerate some code before completing. Will push updates. |
Please see https://github.com/grpc-ecosystem/grpc-gateway/blob/master/CONTRIBUTING.md for the regeneration steps |
Yep, on it. Just had to setup access to docker.pkg.github.com. |
All re-generated now. |
Codecov Report
@@ Coverage Diff @@
## master #1375 +/- ##
=======================================
Coverage 54.14% 54.14%
=======================================
Files 42 42
Lines 4375 4375
=======================================
Hits 2369 2369
Misses 1750 1750
Partials 256 256 Continue to review full report at Codecov.
|
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.
This is great, thanks! My only concern is with the use of the phrase Required
, if it will confuse users. We do have defaults for all the required values, and any of these uses are additive to the defaults. What do you think?
License license = 5; | ||
// Required Provides the version of the application API (not to be confused |
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.
// Required Provides the version of the application API (not to be confused | |
// Required. Provides the version of the application API (not to be confused |
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.
Agreed, I was a little unsure about adding this as obviously there is a difference between what the swagger spec defines as required and what is required for protoc-gen-swagger. I'll remove the required statement. I guess we could document the defaults at some point.
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 looks like there are a number of other descriptions in the proto definition which look like they have been copied from the open api spec verbatim with 'required'. Do you think we should also remove this?
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.
Haha, yeah, just left a comment to this effect.
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.
I've not removed all the other instances of required.
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.
Could we remove the other Required.
as well while we're here please?
Thanks a ton, this is great! Could you please cherry pick this against v2? |
Added PR #1378. Thanks for the review and merge! |
Add missing documentation for openapiv2 proto definition.