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.

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 default value for swagger 200 response #767

Conversation

johnchildren
Copy link
Contributor

Re: #766

This commit adds a default value to the description field of 200
responses. This is needed to meet the swagger specification requirement
of a short description for all response objects.

This commit adds a default value to the description field of 200
responses. This is needed to meet the swagger specification requirement
of a short description for all response objects.
@johnchildren
Copy link
Contributor Author

johnchildren commented Oct 2, 2018

I couldn't see an obvious place to add this in the tests, but they seemed to pass locally regardless of the change. It may be worth also updating the example swagger files as they currently have empty descriptions.

edit: based on CI I was obviously running the wrong command, will look into updating the generated files.

@johanbrandhorst
Copy link
Collaborator

@ivucica would like your opinion on this if possible.

@johanbrandhorst
Copy link
Collaborator

You needn't really bother with tests, it should be good enough that the generated files now have a description set somewhere where it wasn't before.

@ivucica
Copy link
Collaborator

ivucica commented Oct 2, 2018

Regarding justification: I'd argue the spec only requires a string to be set, and not that it's a non-empty string. See relevant section in spec; while it says it's required, it doesn't say what the string should look like.

(For what it's worth, the spec also actually doesn't say what it means by the word "required" in this context; the quoted RFCs use it slightly differently.)

editor.swagger.io does not complain about a description set to an empty string.

But, having said this, I believe the change to be an improvement. LGTM.

On a separate note: line 717 will make the resulting description look ugly, but that's already the case if people specify a documentation comment. I'd remove that case, but that's something to be done in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants