-
-
Notifications
You must be signed in to change notification settings - Fork 352
Split "prefixItems" from "items", drop "additionalItems", make "unevaluatedItems" respect "contains" #925
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
Note also that some of the text regarding how to combine multiple annotation values was intentionally dropped per #906 (Drop the idea of automatically combining annotations). The remaining unevaluatedProperties
text explains how multiple values are to be used for that keyword, and other keywords would provide their own clear conventions as needed.
I will do another PR in the future to take care of the rest of #906 but there was no point in preserving those paragraphs while reworking these sections.
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 seems simpler to just have prefixItems
produce a max index annotation, and items
produce true.
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 the language in items
doesn't seem to account for a true
annotation result from prefixItems
, only numeric
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.
@notEthan if prefixItems
is true then items
doesn't do anything. There's no need for a length check.
I don't know if this is the PR for it, but it would make more sense to me if items, prefixItems, and contains all produced arrays of array indices as their annotations. I'm not fond of the mishmash of true
, max applied index, and array of applied indices which are all now options.
this would be more consistent with the arrays of property names produced by the *properties keywords.
@notEthan The point of the annotations is to allow the most performant creation and utilization of the information. If an instance contains a 500-element array, there is no reason for items
to emit a 500-element array of indices. All you need to know is that all elements have been evaluated. That could be done with a check against the size of the array, but there's no reason to do that check when there is no cost to just set it to true
for items
or unevaluatedItems
prefixItems
might require a check of the prefix length vs total length which is why it's a MAY and not a MUST to use a boolean- I could see dropping the boolean option for prefixItems
although I wouldn't support it. You can file that as an issue if you would like, I do not want to address it here.
all right, conceded that an arbitrarily large array shouldn't emit an equally-large annotation array, I'll drop that idea.
I'm still going to advocate for one annotation type per keyword - items emits true, prefixItems emits a number, contains emits an array. this split seems natural to me as a part of the items
split.
the alleged benefit of prefixItems emitting true is based on the idea that a check for a true value is more efficient than a length check against an integer. I think the performance difference between those is completely negligible, and the added complexity of accommodating multiple type options for prefixItems annotations outweighs any few cycles' difference between comparing booleans and integers.
I could see dropping the boolean option for prefixItems although I wouldn't support it. You can file that as an issue if you would like, I do not want to address it here.
sorry I didn't file a separate issue. but this PR is the one splitting items
, so annotations changed by that split seem very much in scope to me. however I will not pursue it further after this comment if you remain against it.
sorry I didn't file a separate issue. but this PR is the one splitting items, so annotations changed by that split seem very much in scope to me. however I will not pursue it further after this comment if you remain against it.
@notEthan I'm just not doing it right now. I very strongly oppose scope creep on issues [EDIT: I meant PRs]. This PR just splits and carries forward the previously existing annotation scheme (adding annotations for contains
). We spent an epic amount of time sorting this out in the issue and that's done and approved.
If you want to change the pre-existing annotation approach, you need to file an issue on that and not try to expand the scope of a major change that went through a lot of negotiation already, just because it touches the same area.
For me, I need to get PRs completed. Someone else can decide if your suggestion warrants a change in this draft, I'm trying to stay out of that because I have more than enough work to do with the issues already assigned to the draft.
Is the bikeshedding on the "prefixItems" name totally done (vs. alternatives like tupleItems, sequentialItems etc). The spec should make it very clear what "prefix" is referring to -- for example "all items that come at the beginning of the list, and processed before additionalItems considers what items it should evaluate against".
@karenetheridge yes, the naming is 100000000% final. Please see the issue for that whole saga, but it will not be reopened. Or rather, if you want to re-open it, ask @Relequestual and y'all go off and figure it out and tell me what happens 😛 All of my arguments for prefixItems
are in the issue and don't need further elaboration. I was generally fine with tupleItems
but someone had objections there, I think even beyond my concern that the relationship between tupleItems
and items
was not clear from the names.
The dependency of items
(not additionalItems
which has been removed) on prefixItems
is all that needs to be said in terms of order of processing. Technically, since you can implement items
without annotations, you don't actually need to process them fully in that order anyway. It's an implementation detail and does not belong in the spec.
I will add a line about its conceptual relationship with items
, though, that's a good catch. It needs to be clear that you can have a "prefix" without anything following it.
@karenetheridge I agree that using prefixItems
on its own is a little awkward, but so is using additionalProperties
on its own, which is an important use case that people often don't even notice. Naming is hard. [EDIT: I originally wrote (削除) by accident]additionalItems
(削除ここまで)
(削除) @handrews I think maybe you meant "additionalProperties
on its own" ? additionalItems on its own had no effect (and is now gone). (not meaning to nitpick, just to clarify) (削除ここまで)
@notEthan yeah thanks, fixed
@karenetheridge updated the text, see if this is better. I am trying to keep this fairly minimal as keywords specs are not tutorials, but I agree that a bit more explanation was called for.
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 PR should not include contains
changes as we have not resolved that topic.
@gregsdennis take the contains
thing up with @Relequestual in the issue or in slack, and let me know the outcome. I'm not going to bother to disentangle this until there's a decision. The issue was marked block pending the split issue, and that issue got resolved, so I went ahead.
I feel strongly that these should interact, and I don't have anything further to say on it. If you sell @Relequestual on overruling me that's cool, I'll change it then.
This reworks the array applicators in several ways: * "prefixItems" takes on the former role of the array form of "items" * "items" keeps its single-schema role, and takes on the role of "additionalItems" when "prefixItems" is present * "contains" now produces an annotation indicating what it evaluated * "unevaluatedItems" now respects "contains", and the language around interpreting the relevant annotation is (hopefully) less convoluted Note that this does not address the change to put unevaluatedItems into a separate vocabulary with unevaluatedProperties, which will be done as a separate PR.
This starts from the 2nd entry because a pending PR is using the first entry.
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.
Made one suggestion for change. After said change you have my thumbs up.
Just a slight clarification which I feel removes potential ambiguity.
Co-authored-by: Ben Hutton <relequestual@gmail.com>
Uh oh!
There was an error while loading. Please reload this page.
Closes #810 (
unevaluatedItems
should considercontains
)Closes #864 (split
items
)This reworks the array applicators in several ways:
"additionalItems" when "prefixItems" is present
interpreting the relevant annotation is (hopefully) less convoluted
Note that this does not address the change to put unevaluatedItems
into a separate vocabulary with unevaluatedProperties, which will
be done as a separate PR. Doing that will also take care of the current ordering
problem-
contains
should appear beforeunevaluatedItems
, but deferringthat keeps this diff more readable.
It is also possible that questions raised on slack today regarding
contains
/minContains
/maxContains
might further impact this, but any such impacts will be done as a separate, follow-on PR so as not to delay getting this section of work checked in.