-
-
Notifications
You must be signed in to change notification settings - Fork 314
additionalItems meta-schema dependency #209
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
Comments
Yeah, that makes sense. I think it would make some sense to say "items" MUST be an array - that would make things more consistent, so that additionalItems/items can't serve the same purpose. |
@awwright additionalItems and items already can't serve the same purpose. And @epoberezkin's proposed change does require items to be an array if additionalItems is present. What do you want different? |
If I understand correctly, for schema values (when I want to specify what every item in the array must look like), I can use either "additionalItems" to "items" with no difference in behavior. Specifying that "additionalItems" implies "items" MUST be an array seems reasonable. Can we make "items" always be an array? |
No, you cannot. From the spec:
So the only case in which "additionalItems" is not ignored is when "items" is an array. Why should we break how "items" currently works? |
BTW I am not dead set against changing "items", and I agree that the current situation is kinda weird. But it is a big change with a broad impact. It would be easy enough to migrate with a script (which is true of our other recent incompatible changes as well), but it's not a change I'd want to rush into. |
... when items is array |
Not sure if I follow why do we need to change items? What is weird in the current situation? The most common use for items is when items is a schema that is applied to all items. All I propose that in this case additionalItems are prohibited rather than ignored. EDIT: because otherwise some people expect that both schemas will be applied to all items in this case, although I have no idea why they need two schemas... |
*HEADDESK* x 9000 :-P (I went back and edited it, thanks)
Oh good to know. I mean, they can just use "allOf" with two "items", but people don't always think of that. The more I think about it the more it would actually make sense to have "items" be only an array and always use "additionalItems" for the schema. It would be more symmetric with "properties" (for enumerating properties) and "additionalProperties" (for matching everything that was not enumerated). We are already doing incompatible changes for draft 6 so... yeah, maybe now is the right time? We've no doubt got a few weeks to gather feedback before draft 6 goes out the door at minimum, so perhaps that's enough. |
Nope. Bad idea. Properties are usually validated individually, different properties more often have different schemas. Items, on the other hand, usually have the same schema. Having to write additionalItems all the time when you are just validating items is a very bad idea. They would be additional to what? Items as array is a rare case, so restricting items to a rare case is not good. |
We could use "items" for schema only, drop "additionalItems", and add a new keyword, "itemTuple" or something, for the array form. (I'm not super-attached to any of this, just poking at the idea now that it's been brought up) |
OR... "items" is a schema always applies to all elements and maybe "additionalItems" can only be present if "itemTuple" is present? |
@handrews why can't we have things as they are? i.e. have both object and array form for items keyword? I always found it elegant and was thinking that something like this for properties could be interesting too (never was able to write down anything sensible, so don't freak out please). It's just "ignored" business that I don't like - I think it's alway better to fail hard than to ignore something... |
@epoberezkin since @awwright proposed changing things ( #209 (comment) ), I wanted to give that consideration. As I mentioned in response to his comments, I'm fine with things as they are. |
Ok. I see. I actually missed that part of that comment, sorry @awwright. Anyway, I can only repeat that "items" as array is a small fraction of use cases, so using "additionalItems" all the time is verbose and confusing. I don't think we need to change the existing syntax. Still the original suggestion (only allow "additionalItems" when "items" is array) would reduce the confusion. And it would be semantically consistent with the keywords: "items" validates all items, "additionalItems" only validates beyond defined items. We don't need to make array validation vocabulary consistent with object validation, they are different structures with different use cases. |
@awwright, @epoberezkin is anyone still trying to get anything changed with this issue? I just read through it and can't really tell. |
No one answered my question about whether anything still needs to be done so I'm closing this. I'm personally very reluctant to forbid keywords. We have a very well established pattern that keywords that don't make sense are ignored, rather than causing errors. That is very important for extensibility, and I would rather not break that pattern. Particularly since my last comment sat for five months with no response. |
To follow up from a discussion with @epoberezkin, I would prefer to leave this loose as that is our general design pattern. However, I would like to make it easier to extend the meta-schema (for instance by making a stricter one) while still identifying the underlying vocabulary. See #314 (which I will update with this and other use cases). If that idea ends up not working out, then we can re-visit this issue. |
At the moment the spec says:
That causes a lot of confusion from users. The suggestion is to prohibit additionalItems (rather then require ignoring it) and enforce it via meta-schema unless items is present and it is an array.
The meta-schema will be inside dependencies, together with exclusiveMaximum and exclusiveMinimum (#189 (comment))
The spec also becomes simpler:
@awwright @handrews @Relequestual if that makes sense, I can make a PR
The text was updated successfully, but these errors were encountered: