-
-
Notifications
You must be signed in to change notification settings - Fork 292
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
Keywords can depend on subschema or adjacent keyword annotation results #600
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.
LGTM overall. Just a few questions inline.
jsonschema-core.xml
Outdated
<t> | ||
Annotation results from multiple "items" keyword are combined | ||
by setting the combined result to true if any of the values | ||
are true, and otherwise retaining the larges numerical value. |
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.
larges -> largest
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 swear I run a spell-checker...
thanks
jsonschema-core.xml
Outdated
index of the instance, such as when "items" is a schema. | ||
</t> | ||
<t> | ||
Annotation results from multiple "items" keyword are combined |
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.
multiple "items" keyword
is a bit surprising. Do you mean, e.g., having several "items" keyword present in elements of an "allOf"?
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.
Yes, need to improve the language here, thanks.
What's a good shorthand for "several X keywords present in elements of allOf
/anyOf
/if
/etc."?
{
"anyOf": [
{
"items": [{"type": "integer", "multipleOf": 2}, {...}, {...}]
},
{
"items": [{"type": "integer", "multipleOf": 3}, {...}, {...}, {...}]
},
{
"items": {"type": "string"}
}
]
}
The rolled-up annotation from "items" might end up as 3 (if it's a multiple of 2 but not of 3) or 4 (if it's a multiple of 3, regardless of whether it's a multiple of 2). Or true
if the first element is a string. Or there wouldn't be an "items" annotation if the first element is neither a string nor an integer that is divisible by either 2 or 3.
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.
What's a good shorthand for "several X keywords present in elements of allOf/anyOf/if/etc."?
Maybe "several X keywords collected by application of a combination keyword", where "combination keyword" would link to section Keywords for Applying Subschemas in Place? Just picked-up the "combination" term, as I think we don't have a word yet for this group, maybe there's a better one...
Does that help?
Yes, thanks.
@@ -1240,26 +1243,52 @@ | |||
same position, if any. | |||
</t> | |||
<t> | |||
Omitting this keyword has the same behavior as an empty schema. | |||
This keyword produces an annotation value which is the largest | |||
index to which this keyword applied a subschema. The value |
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.
Why is the result the index of the last applied item rather than the list of indices of applied items? This looks unsymmetrical with how it works for "properties". (If this something that got discussed before, I probably missed it, so any pointer or clarification would be welcome.)
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.
Because it's not possible to have gaps in the list. There could be if contains
were used, but as noted in that section, contains
is not used. And if it were, then its annotation would be a list of indices, but it still wouldn't be possible for items
to ever produce a gap, so why bother? Particularly if the list is very long.
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.
Ok, makes sense, thanks!
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 think I see anything wrong with this, but I'm trying to think if there's a way this can be made more compact.
Good luck and let me know if you can think of anything. This is where I got after editing stuff down a lot, but I'm sure it could be improved. |
an empty schema. | ||
</t> | ||
<t> | ||
Implementation MAY choose to implement or optimize this keyword |
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.
Note to self: typo -"Implementations"
@awwright I should note that I expect to do a big editing pass towards the end of the draft work. Once we get all of the new material into the core spec, the optimal organization will be more clear. And that will help me edit the wording down. Right now I'm not worrying too much about it, which is why "Overview" has become rather enormous. But reorganizing and condensing things is not something that I can do piecemeal, I kind of need all of the content to be present before I can really see the patterns and use them to reduce the text. So while I'm definitely interested in improvements, I'd rather not hold this up just for verbosity. We can file an issue to track doing that editing pass to make sure I don't forget, if you'd like. |
Describe how default behaviors work in more detail, covering all three major keyword categories. Allow keywords to depend on the presence or absence of adjacent keywords (this is already true with "additionalItems" and "items'). Allow keywords to depend on the annotation results of subschemas and adjacent keywords. This is the fundamental mechanism behind the forthcoming "unevaluatedProperties" and "unevaluatedItems" keywords. The behavior of "additionalProperties" and "additionalItems" can be described with this mechanism (along with the presence or absence mechanism) as well, which will serve as a proof of concept of the approach in a subsequent commit.
These paragraphs are no longer needed now that the keywords are clearly designated as Applicators. Both the general applicator section and the introduction to this vocabulary cover the behavior clearly. Note that the remaining phrasing is still too validation-centric. This will be addressed in the future.
This aligns the behavior of "additionalItems" and "additionalProperties" with the rules given for dependent keywords in the previous commit. Specifically, "additionalItems" depends on the presence or absence of "items", and its newly defined annotation results, while "additionalProperties" depends on the annotation results of "properties" and "patternProperties". The standard explicitly allows implementing these keywords in other ways (presumably the way implementations currently handle them) as either an optimization or to support the keywords in implementations that do not support annotations. This avoids requiring existing assertion-only validators to change their behavior. Note that the rationale for "additionalItems" being ignored when "items" is absent has been changed to align it with this new framework. Doing anything else would have required an absent keyword to produce annotation results as default behavior, which leads to very difficult problems as the set of keywords with such behavior grows.
This tries to make the concept of having multiple annotation results for the same keyword more clear in the applicator vocabulary.
OK, @dlax I tried to improve the wording around the multiple annotations per keyword thing. @awwright that made it even more verbose, but as noted I'd like to settle for clarity at this stage and then focus on brevity later. I suspect that as we fill out and organize the front matter, some of the things that currently need to get re-explained for each keyword will be more clear overall, so we should be able to trim the per-keyword text. |
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 makes enough sense to me, sure
Keywords can depend on subschema or adjacent keyword annotation results
The base of this PR is PR #595, not the current master
This is the critical conceptual change that lets us implement #556
unevaluatedProperties
and #557unevaluatedItems
.There are three commits- two significant one and one mechanical one removing outdated text. These are grouped together to ensure that a real-world use of the new concepts, with the familiar
additionalProperties
andadditionalItems
keywords, is included with the more abstract changes.The first commit describes the newly allowed behavior and its implications for all types of keywords. In particular, this required working out how default keyword behavior works with respect to annotations.
The second commit just removes outdated boilerplate that I meant to remove when I moved the applicators from validation to core. Now that all applicators are grouped into a vocabulary, we don't need to re-explain that concept on each keyword.
The third commit reworks the
*item
and*properties
keywords to use the new approach of depending on annotation behavior. Implementations are not required to code this approach exactly, as the current ways of implementing these keywords are faster (and implementations that do not opt-in to annotation collection still need to support these keywords!)items
,properties
, andpatternProperties
now produce annotation results indicating what indexes / property names they cover, andadditionalItems
andadditionalProperties
now define their behavior in terms of those annotation results. They, too, produce annotation results about what they cover- those results will be used byunevaluatedItems
andunevaluatedProperties
.