Skip to content
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

Split "prefixItems" from "items", drop "additionalItems", make "unevaluatedItems" respect "contains" #925

Merged
merged 4 commits into from
May 31, 2020

Conversation

handrews
Copy link
Contributor

@handrews handrews commented May 15, 2020

Closes #810 (unevaluatedItems should consider contains)
Closes #864 (split items)

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. Doing that will also take care of the current ordering
problem- contains should appear before unevaluatedItems, but deferring
that 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.

@handrews handrews added the core label May 15, 2020
@handrews handrews added this to the draft-08-patch1 milestone May 15, 2020
@handrews handrews requested a review from a team May 15, 2020 18:32
@handrews
Copy link
Contributor Author

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.

schemas applied to the same instance location are combined
by setting the combined result to true if any of the values
are true, and otherwise retaining the largest numerical value.
index of the instance, such as is produced by the "items" keyword.
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

@notEthan
Copy link
Contributor

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.

@handrews
Copy link
Contributor Author

@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.

@notEthan
Copy link
Contributor

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.

@handrews
Copy link
Contributor Author

handrews commented May 16, 2020

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.

@karenetheridge
Copy link
Member

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".

@handrews
Copy link
Contributor Author

@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.

@handrews
Copy link
Contributor Author

handrews commented May 19, 2020

@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 additionalItems by accident]

@notEthan
Copy link
Contributor

notEthan commented May 19, 2020

@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)

@handrews
Copy link
Contributor Author

@notEthan yeah thanks, fixed

@handrews
Copy link
Contributor Author

@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.

Copy link
Member

@gregsdennis gregsdennis left a 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.

@handrews
Copy link
Contributor Author

handrews commented May 21, 2020

@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.

handrews added 3 commits May 22, 2020 12:41
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.
jsonschema-core.xml Outdated Show resolved Hide resolved
Copy link
Member

@Relequestual Relequestual left a 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split items into items and tupleItems "unevaluatedItems" should consider "contains"
5 participants