-
-
Notifications
You must be signed in to change notification settings - Fork 324
re-unite meta-schema #1610
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
base: main
Are you sure you want to change the base?
re-unite meta-schema #1610
Conversation
}, | ||
"patternProperties": { | ||
"^x-": true, | ||
"^(?!(?:\\$id|\\$schema|\\$ref|\\$anchor|\\$dynamicRef|\\$dynamicAnchor|\\$comment|\\$defs)$)\\$.*": false |
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.
Isn't this superfluous?
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.
Karen suggested an improvement.
It's needed. The updated spec restricts the definition of custom $*
keywords. The only way to do that is to explicitly allow the ones you have.
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 think the meta-schema should adhere to jsonschema-core's "given the high disparity in regular expression constructs support, schema authors SHOULD limit themselves to the following regular expression tokens: ...".
Negative lookahead is definitely a feature with much disparity in support.
But I don't know another way to express this constraint for patternProperties
.
Maybe propertyNames
would be better suited to validate this?
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.
good catch. Maybe this is one of those requirements where we just don't enforce it with the meta-schema. I'm okay with that.
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 don't know another way to express this constraint for
patternProperties
.
Would you be willing to do it with additionalProperties
?
"^(?!(?:\\$id|\\$schema|\\$ref|\\$anchor|\\$dynamicRef|\\$dynamicAnchor|\\$comment|\\$defs)$)\\$.*": false | |
"^([^$].*)?$": true | |
}, | |
"additionalProperties": false, |
"^([^$].*)?$": true
in patternProperties
allows property names that are either the empty string or start with a character other than $
, making "additionalProperties": false
apply exclusively to (and reject) properties that lack an explicit mention in properties
and start with $
.
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.
Looks good. Thanks for doing this work.
Left one comment, but not essential.
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.
Looks good to me
"$schema": "https://json-schema.org/version/year/meta", | ||
"$id": "https://json-schema.org/version/year/meta", |
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.
What's "meta"? (Sorry if it's something we discussed and I forgot.)
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.
Also, what's the reason for using a placeholder for "version" and "year"?
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.
What's "meta"?
it was late. It should just be /version/year
Also, what's the reason for using a placeholder for "version" and "year"?
It's not published yet. Just wanted to communicate the idea.
"^x-": true, | ||
"^(?!(?:\\$id|\\$schema|\\$ref|\\$anchor|\\$dynamicRef|\\$dynamicAnchor|\\$comment|\\$defs)$)\\$.*": false | ||
}, | ||
"$defs": { |
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 appears to be missing a definition for "stringArray".
}, | ||
"patternProperties": { | ||
"^x-": true, | ||
"^(?!(?:\\$id|\\$schema|\\$ref|\\$anchor|\\$dynamicRef|\\$dynamicAnchor|\\$comment|\\$defs)$)\\$.*": false |
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 don't know another way to express this constraint for
patternProperties
.
Would you be willing to do it with additionalProperties
?
"^(?!(?:\\$id|\\$schema|\\$ref|\\$anchor|\\$dynamicRef|\\$dynamicAnchor|\\$comment|\\$defs)$)\\$.*": false | |
"^([^$].*)?$": true | |
}, | |
"additionalProperties": false, |
"^([^$].*)?$": true
in patternProperties
allows property names that are either the empty string or start with a character other than $
, making "additionalProperties": false
apply exclusively to (and reject) properties that lack an explicit mention in properties
and start with $
.
What kind of change does this PR introduce?
Refactor
Issue & Discussion References
Summary
Re-unites the meta-schema into a single file since vocabularies are a proposal now. We will likely split it back up again when we figure out vocabs, but for now it doesn't make sense to have them separated.
Also:
x-*
keywords$*
keywordsDoes this PR introduce a breaking change?