Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adding support for default and required swagger annotation fields. #852

Merged
merged 5 commits into from
Jan 22, 2019
Merged

Adding support for default and required swagger annotation fields. #852

merged 5 commits into from
Jan 22, 2019

Conversation

fahernandez
Copy link
Contributor

Solves issue #851

johanbrandhorst
johanbrandhorst requested changes Jan 17, 2019
View reviewed changes
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent start, could you please add some more examples with more than one required field to show how it works in that case?

@johanbrandhorst
Copy link
Collaborator

It looks like this change breaks defaults when the value is unset as well - I think we need to make sure we maintain the current behavior where a default isn't specified.

fahernandez
fahernandez commented Jan 17, 2019
View reviewed changes
Copy link
Contributor Author

@fahernandez fahernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? in the definition of a_bit_of_everything.proto there are several cases where the default field is not defined and the swagger files are generated without problem, which case are you seeing?

@johanbrandhorst
Copy link
Collaborator

See my last comment, enum fields had their default set to the first value previously.

@codecov-io
Copy link

codecov-io commented Jan 17, 2019 •
edited
Loading

Codecov Report

Merging #852 into master will increase coverage by 0.04%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #852      +/-   ##
==========================================
+ Coverage   51.83%   51.87%   +0.04%     
==========================================
  Files          39       39              
  Lines        3764     3778      +14     
==========================================
+ Hits         1951     1960       +9     
- Misses       1629     1633       +4     
- Partials      184      185       +1
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/template.go 48.33% <66.66%> (+0.24%) ⬆️

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 20e8cf9...b76f6a0. Read the comment docs.

johanbrandhorst
johanbrandhorst requested changes Jan 17, 2019
View reviewed changes
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add an example which contains several entries in the required field?

@johanbrandhorst johanbrandhorst merged commit 9221822 into grpc-ecosystem:master Jan 22, 2019
@johanbrandhorst
Copy link
Collaborator

Thanks for this contribution!

@johanbrandhorst johanbrandhorst mentioned this pull request Feb 18, 2019
Closed
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
@johanbrandhorst
Adding support for default and required swagger annotation fields. (g…
fb01528 
…rpc-ecosystem#852)

* Adding support for default and required swagger annotation fields:Solves issue grpc-ecosystem#851

* Fixing test for swagger generation

* updating swagger definition

* removing breaking changes over enums

* Adding example for required field as an array
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanbrandhorst johanbrandhorst johanbrandhorst approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fahernandez @johanbrandhorst @codecov-io @googlebot