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

Fix missing schema type in YAML example spec #1923

Merged

Conversation

Projects
None yet
4 participants
@sjaensch
Copy link
Contributor

commented May 10, 2019

These were fixed for the JSON version of the examples back in mid 2015, but some are still missing for the YAML versions. This is the root cause for Yelp/bravado#416.

@darrelmiller

This comment has been minimized.

Copy link
Member

commented May 16, 2019

The problem is that the absence of the type is not an error. Type is not a required property in a schema. By fixing the examples we lead people down the path of assuming type will always be there, when it will not.

@sjaensch

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

The problem is that the absence of the type is not an error. Type is not a required property in a schema. By fixing the examples we lead people down the path of assuming type will always be there, when it will not.

I think that's a bit besides the point - the YAML spec is different in content and meaning from the JSON spec now (example: the JSON version doesn't allow non-object types, the YAML spec does). They should be identical in content and meaning.

There might be a separate issue here in how bravado recognizes its models.

@webron

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@sjaensch we just talked about on the call and explained @darrelmiller why the type should completely be there. We did make one minor change to your PR to make it a bit more consistent, hopefully you won't object to it, but if you do, please let us know.

@webron

webron approved these changes May 16, 2019

@jstoiko

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Also, if you don't mind @sjaensch, could you make those same changes in the OAS 3.0 version of those examples (i.e. in examples/v3.0/yaml/)? I could have made a separate PR like I had volunteered to do during the tsc call but I now feel too lazy to do so (and it really belongs in the same PR anyway :) ).

@darrelmiller darrelmiller merged commit f1852bd into OAI:master May 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jstoiko added a commit to jstoiko/OpenAPI-Specification that referenced this pull request May 16, 2019

@sjaensch sjaensch deleted the sjaensch:fix-missing-object-type-yaml-examples branch May 17, 2019

@sjaensch

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@jstoiko sure thing! Since this PR has been merged I opened #1931.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.