-
-
Notifications
You must be signed in to change notification settings - Fork 217
When $ref is present other keywords should be ignored #142
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
I was sure there was a test covering this already but it appears there isn't... thanks for submitting. Leaving some comments. |
@@ -141,6 +141,32 @@ | |||
] | |||
}, | |||
{ | |||
"description": "ref, containing other keywords", |
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.
Would be a bit clearer I think to say something like "ref overrides any sibling keywords" which is the behavior, rather than just describing the situation without explicitly pointing that out.
"properties": { | ||
"foo": { | ||
"$ref": "#/definitions/reffed", | ||
"type": "integer" |
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.
Think it also might be clearer to pick an example that isn't just conflicting keywords. E.g., using type array
behind a $ref
but having a minLength
will ignore the minLength
sibling.
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.
But that's true anyway, isn't it?
"properties": {
"foo": {
"type": "array",
"minLength": 10,
}
}
validates "foo": []
and "foo": [1, "a", {}]
but not "foo": "short"
or "foo": "thisiswaytoolong"
because "minLength"
can only fail validation against string instances. It has no effect on other types.
"valid": true | ||
}, | ||
{ | ||
"description": "remote ref valid", |
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.
invalid
7ed7f50
to
e31fce9
Compare
Better? Is this what you meant? |
Yes, nice work! Merging. |
See answer by @Julian on #129