-
Notifications
You must be signed in to change notification settings - Fork 12
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
Move schemas to the root of the json #99
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Joan López de la Franca Beltran <[email protected]>
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.
Imho, would be good to add at least few more test cases (with bit more complex cases of schema refs) but agree (and prefer) to merge this as-is and iterate later.
So, I'd say it LGTM! Although I'd wait for @sdboyer approval cause as I paired on part of these changes, I don't think I count as an actual reviewer.
From a JSON schema perspective I would expect an object with an {
"$id": "http://example.com/schemas/0.0/team.json",
"$schema": "http://json-schema.org/draft-04/schema#",
"$defs": {
"memberCount": { "type": "integer" }
},
"type": "object",
"required": [
"name"
],
"properties": {
"name": {
"description": "Name of the team.",
"type": "string"
},
"count": {
"$ref": "#/$defs/memberCount"
}
}
} This is more cleanly explained here: http://json-schema.org/understanding-json-schema/structuring.html |
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 with @Duologic that a $defs
output is probably preferable, something along the lines of what he's output there.
I read into this a bit and got a bit stuck because it appears the $defs
convention was only introduced in JSON Schema draft...10 i think? I can't remember exactly, but certainly later than draft 4/what we've been targeting here.
However, it's still valid in Draft 4, so in the absence of another convention, it's still probably the best way to organize the outputs.
This does shift the goalposts a bit on what we're actually trying to accomplish with hoisting schemas to the root. It deserves an issue with a more careful, precise writeup.
I'm hoping i can get to that in the latter half of this week.
is fixing this a blocker for fully automating our pipelines in grok, at least for grafonnet? |
I guess it's better to illustrate what this PR does with an example:
before this PR schema is defined under
$.components.schemas
This change moves the schema definition to the root of the JSON: