-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
$recursiveRef part 3 of 3: Update meta-schemas #655
Conversation
@@ -55,13 +57,13 @@ | |||
}, | |||
"$defs": { | |||
"type": "object", | |||
"additionalProperties": { "$ref": "#" }, | |||
"additionalProperties": { "$recursiveRef": "#" }, | |||
"default": {} | |||
}, | |||
"definitions": { |
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.
Although not part of this PR, specifically, can definitions
just redirect to $defs
rather than repeating the subschema? Or is there some deeper purpose to having it explicit?
Maybe something like
"definitions": {
"$recursiveRef": "#/properties/$defs",
"$comment": "look at how well keywords fit right next to $refs!"
}
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 kind of feel like best practices is for refs to target #/$defs/..., not #/properties/..., and it seems overkill to factor this out.
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.
That's fair, but it also feels redundant to have a definition that is truly supposed to be the same thing in here twice. Maybe move the whole thing to a definition and have both properties reference it
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 like the explicitness more than I am concerned about a 3-line redundancy here.
schema.json
Outdated
"propertyNames": { "format": "regex" }, | ||
"default": {} | ||
}, | ||
"dependentSchemas": { |
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 rather like this being explicitly defined as a schema dependency vs a property dependency. Why are you changing back to just dependencies
?
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.
That was unintentional and I have no idea, thanks for catching. I blame coming back to this after three months and probably my tree was in a weird state.
schema.json
Outdated
{ "$ref": "#/$defs/stringArray" } | ||
] | ||
} | ||
"propertyNames": { |
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 change doesn't fit the wording of the propertyNames
keyword.
The value of "propertyNames" MUST be a valid JSON Schema.
If the instance is an object, this keyword validates if every property name in
the instance validates against the provided schema.
Note the property name that the schema is testing will always be a string.Omitting this keyword has the same behavior as an empty schema.
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 wonder if there's a way that we can write a schema that validates schemas that validate strings. That would be perfect here.
{
"allOf": [
{ "$recursiveRef": "#" },
{
"type": "object",
"properties": {
"type": {"const":"string"}
},
"required": [ "type" ]
}
]
}
Concise? No. Explicit? Yes. Necessary? ¯\_(ツ)_/¯
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 change was also unintentional. Generally, we make the meta-schema more permissive rather than trying to lock it down. We've gone over similar changes before and always decided not to do them. It's totally valid to write propertyNames
without type (JSON is not capable of serializing anything else, so its utility is debatable), or even with the wrong type (although I have no idea why, or what useful thing it could do). There's a similar restriction with hrefSchema
that it probably should have type: object
but we don't enforce that either.
hyper-schema is a lot simpler now.
2e973df
to
8d0b831
Compare
OK, apparently I grabbed a really old meta-schema for this change originally somehow. I've fixed it so it no longer reverts the last few other changes :-/ I left the change trimming |
hyper-schema is a lot simpler now.