-
-
Notifications
You must be signed in to change notification settings - Fork 315
Improve child validation wording from feedback. #193
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
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.
Other than my one comment, this looks good.
For arrays, primitive type validation consists of validating | ||
restrictions on length with "minItems" and "maxItems", while | ||
"items" and "additionalItems" control which subschemas apply | ||
to which elements of the array. In addition, "uniqueItems" |
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 find the phrasing
...control which subschemas apply to which elements of the array.
a little confusing. It may be just my lack of understanding though. Can you expand on this section please?
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.
Those keywords tell you whether a given subschema applies to a given child instance or not.
This is the critically important point that differentiates these keywords from all others. Rather than applying rules to the immediate instance and producing a validation rule based on that, these keywords just say "the validation outcome of this keyword is simply the result of applying the subschema to certain child instances", and give you rules on which child instances are affected by the subschema.
Does that help?
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.
It's extremely important because implementing these is different from implementing other keywords. There are basically four types of validation keywords:
- Immediate instance keywords (most of them)
- Apply-subschema-to-child-instance keywords (these)
- Logical combinations of schema results ("allOf"/"anyOf"/"oneOf"/"not"/"dependencies"/"if"-"then"-"else" assuming it eventually goes in)
- Array content keywords ("uniqueItems" and "contains") that can only be evaluated by looking at the entire array and all of its contents
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.
It also highlights how these keywords (like "allOf"/"anyOf"/"oneOf"/"not") are important for more than just validation. All other vocabularies, such as hyper-schema, are pretty much useless without these keywords as they are the only way to apply other vocabularies to complex instances. The core vocabulary ("$schema", "$id", "$ref") does not provide any such mechanism, and vocabularies such as hyper-schema should not re-implement the process. These keywords and the logical keywords have implications far beyond validation.
@Relequestual I have changed the "controls" phrasing, replacing the word with "determines" and changing the sentence structure a bit to (hopefully) make it all more clear. How does this look now? |
@awwright you assigned this to yourself a month ago, so it's blocked on you. What is needed to get your approval here? |
"required", "minProperties", "maxProperties", "propertyNames", | ||
and the string array form of "dependencies", while "properties", | ||
"patternProperties", and "additionalProperties" determine which | ||
subschemas apply to which object property values. |
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'd consider adding "... and do not directly validate the instance itself.", after reading your explanation at #161 (comment) I found your phrasing there explaining...
I recently rewrote the items, additionalItems, properties, patternProperties, and additionalProperties to make it clear that they do not directly validate the instance, but instead control which subschemas are applied to which child instances.
to actually be a clearer explanation that the changes you've made.
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.
@Relequestual I have made the changes you requested and replaced "validation always succeeds" with "does not directly validate the instance itself."
Sorry for being a blocker. Thanks for calling me out. Edit: |
This implements suggestions from discussions in issue json-schema-org#161.
This implements suggestions from @deuberger discussions in issue #161.