-
-
Notifications
You must be signed in to change notification settings - Fork 320
Split "unevaluatedItems" and "unevaluatedProperties" into their own vocabulary. #981
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
This change just moves things to the right new section, which includes some vocabulary section boilerplate. Further changes will modify the contents of the moved sections appropriately.
No longer need exceptional case for individual keywords that require annotation collection.
This attempts to address various points of confusion noted in feedback on the most recent draft.
Leaving them as 2019-09 for now as we'll update that all at once.
This comment has been minimized.
This comment has been minimized.
Not sure why I put this before the section defining the concept of "keywords", clearly it belongs in that section.
Now that these keywords depend on annotation results from another vocabulary, the evaluation order of the vocabularies is now significant, so it should be explicitly stated what that ordering should be (e.g. this vocabulary always runs last after all others defined by the core specification). |
@karenetheridge Want to file a new issue for that? This PR is about to be merged. |
@Relequestual @karenetheridge #995 (which for some reason still has no comments despite addressing something that has been brought up repeatedly in the last few weeks... is this thing on?) |
These instance items or properties may have been unsuccessfully evaluated | ||
against one or more adjacent keyword subschemas, such as when an assertion | ||
in a branch of an "anyOf" fails. Such failed evaluations are not considered | ||
to contribute to whether or not the item or property has been evaluated. | ||
Only successful evaluations are considered. |
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've been thinking about this section and its phrasing.
It's the first time in the whole document we mention "evaluation", and we don't really define what it means.
People (including myself previously) incorrectly assume that if a location has been evaluated (had something applied to it), then it counts as having been evaluated.
I'm going to riff some ideas...
We say here "successful evaluations", we mean if an application doesn't result in validation errors, and therefore the applied schema or keyword is correctly applicable to the instance location.
For example, if you had a schema...
anyOf [ { title> unicycle, props > wheels > const > 1 }, { title: bicycle, props > wheels > const > 2 } ]
If your instance was a unicycle, the first subschema item in the anyOf
would be applied and successfully evaluated. Because it only has one wheel, we know it's a unicycle. It is not a bicycle, so that whole subschema is not successfully evaluated.
The instance is successfully evaluated as (or determined to be) a unicycle.
We can further justify this terminology by the fact that evaluation can be short-circuited, and therefore there's no expectation for a schema to be fully evaluated (although that would be a weak argument, and probably confuse things).
We need to clearly define what successful evaluation means, and what unsuccessful evaluation means, with justification (IMHO). I'm not currently convinced I can do that.
I'm happy for this to be a new issue in order to get this PR merged, but it would be essential to 2020-NN.
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 is a good point, and it should be made clear that these keywords really mean "unSucessfullyEvaluatedThing" rather than just "unEvaluatedThing".
yokay, I'm still reading the rest of the issue. If I see no problems, it's merging time. Edit: waiting for reply on my in code comment |
Let's make it a new issue to get this merged and then I can reply to that new issue in the next few days. |
Per conversation with @Relequestual on chat I've file #998 and am going to go ahead and merge this. |
Fixes #853. Fixes #831.
Quite some time ago we agreed to split these out. Now support (or lack of support) is all handled through
$vocabulary
, without any "the vocabulary is supported except these keywords might error out if the implementation just didn't feel like it" behavior.If the implementation doesn't support annotations, it won't be able to process anything that requires the unevaluated vocabulary.