-
-
Notifications
You must be signed in to change notification settings - Fork 306
Reconsider allowing "contains" to apply to objects #1358
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
We should follow the example we set with |
Creating |
Just thinking of maintaining symmetry. |
Yes let's take a close look at this, as far as I can tell, this can definitely break some schemas, and makes some types of schemas more difficult to write. And what's worse, it's very difficult to look at a schema (this keyword by itself, at least) and determine which behavior the author intended. It's also quite unusual for keywords to apply to more than one type of value (but not all of them). In this case, it now can invalidate/reject object or array instances, but not other types...? This impacts schemas such as:
It seems this change would break many instances of the form |
I'm ok with changing it to be two keywords. If we do that, I agree that the array version should remain |
My perfectionism is twitching that these keywords will be asymmetric: Pendantically, |
During yesterday's meeting, I brought up that in the time since we decided to allow
"contains"
to apply to objects, we've shifted to more of an emphasis on stability."contains"
was added in draft-06, and while the details of its interactions with"minContains"
and"maxContains"
(added in 2019-09) have changed, its assertion behavior on its own has not. Allowing these three keywords to apply to objects is a breaking change.We could instead revert them to apply only to arrays and introduce keywords named something like
"containsPropertyValue"
,"minContainsPropertyValue"
,"maxContainsPropertyValue"
alongside them. I'll let others sort out the ideal names."containsProperty"
might be fine but also feels a bit like it is talking about property names rather than values. Another option could be"hasValue"
or"hasPropertyValue"
or something of that sort. All of the name options are less aesthetically appealing than having"contains"
work on objects, but they'd preserve more stability.I don't have an incredibly strong opinion on this, but it seemed to get some traction in the call so I thought I'd file it.
The text was updated successfully, but these errors were encountered: