Allow an empty array for "required". #112
Conversation
|
Paging everyone on issue #69: @awwright @Relequestual @gibson042 @epoberezkin @erosb This seemed uncontroversial by the time the discussion stopped. Any three out of the five of you willing to give approval so this can get merged? |
|
I agree. But why don't we allow array in dependencies to be empty too in this case? For consistency sake? |
Feel free to make another pull request, or I can make one after this is accepted, but please don't block this one because it doesn't do more stuff. The issue was about allowing an empty "required" array. We should still do this even if someone else objects about dependencies. |
I doubt my approval matters, but +1. |
|
Okey, +1 |
| @@ -117,7 +119,7 @@ | |||
| "additionalProperties": { | |||
| "anyOf": [ | |||
| { "$ref": "#" }, | |||
| { "$ref": "#/definitions/stringArray" } | |||
| { "$ref": "#/definitions/nonEmptyStringArray" } | |||
There was a problem hiding this comment.
Why is "dependencies" being modified here?
There was a problem hiding this comment.
Search and replace error, good catch. (although some want this done, I'd rather keep this focused on requirements unless there's immediate agreement to do it for dependencies as well- otherwise I'll fix this to just be for requirements as intended).
There was a problem hiding this comment.
Oh, wait, actually this is correct. I decided that "stringArray" made more sense as an unrestricted array of strings, and then re-named the one with a requirement of at least one element nonEmptyStringArray.
So this actually does not change the behavior of dependencies. It is the same, but requirements is changed because plan "stringArray" was redefined.
For people who will be reading this for the first time, I think this is more clear, but I can flip it if you prefer.
There was a problem hiding this comment.
I think this change makes sense.
There was a problem hiding this comment.
We could also consider if both dependencies and required should allow any number of items.
They effectively do the same thing, after all.
|
I'd like to just modify "stringArray" so both "dependencies" properties and "required" allows multiple items. They're both used for the same purpose, we should probably modify this behavior together. |
|
@awwright @epoberezkin dependencies now also allows for an empty array, which makes all string array definitions consistent. |
|
Should the wording make clear that the array may be an empty array? |
This addresses the enhancement requested in issue json-schema-org#69. The "requirements" and "dependencies" string arrays should be allowed to be empty, with the same effect as not being present.
Done, for both keywords. |
This addresses the enhancement requested in issue #69.