-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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/theme json schema validation #44799
Conversation
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 all the JSON Schema fixes!
@@ -1597,6 +1601,8 @@ | |||
"spacing": {}, | |||
"typography": {}, | |||
"filter": {}, | |||
"shadow": {}, | |||
"outline": {}, |
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.
*sigh* I wish there was a better way in JSON schema draft-04 to add properties than updating 5 different places in a nearly 2000 line file, but unless we start generating the schema from our own metadata, we'll just have to keep fixing issues like these.
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.
Thank you for the review!
For example, if we only consider what is required of a JSON schema (validation and autocompletion), we should easily be able to define the following:
"stylesPropertiesComplete": {
"type": "object",
"allOf": [
{
"$ref": "#/definitions/stylesProperties"
},
{
"properties": {
"border": {},
"color": {},
"spacing": {},
"typography": {},
"filter": {},
"shadow": {},
"outline": {}
},
"additionalProperties": false
}
]
},
to...
```json
"stylesPropertiesComplete": {
"type": "object",
"allOf": [
{
"$ref": "#/definitions/stylesProperties"
},
{
"additionalProperties": false
}
]
},
But since the current implementation of the document generator only takes properties
into account, I think there is always a need to do a double definition.
I wish I could update the logic to generate the appropriate documentation, taking into account ref 😅
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.
Unfortunately the second one doesn't work as JSON schema validates the sub-schemas in allOf
independently. It would validate the types of all of the stylesProperties
which would pass, and then it would validate { "additionalProperties": false }
which says that there should be no properties which would always fail.
It's a limitation of JSON schema in general, and it's one of the reasons some projects, like the tsconfig JSON schema, genreate the schema from metadata instead of writing it by hand.
EDIT: I should have been more specific in my initial comment rather than just complaining. And this isn't to say that we should do our schema exactly like TypeScript—we have our own unique project constraints. All I know is that as long as we're writing the schema by hand, we're going to keep running into issues like this. I don't have the capacity to work on such a system right now, but it's on my wish list. Until then, I appreciate the fixes you've been making 🙂
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.
Thank you for the detailed explanation. If there is anything I can do, I will assist you 👍
Fix: #44442
What?
This PR solves the problem that the three properties
filter
,shadow
, andoutline
are not defined correctly in the theme.json schema.Why?
This is because these three are not correctly defined in the
properties
section that is the source of the validation.How?
I updated
filter
,shadow
, andoutline
in a consistent order, besides adding the definitions.Testing Instructions
theme.json