-
-
Notifications
You must be signed in to change notification settings - Fork 358
schema for $ref keyword #211
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
fd9c0e1 to
eaaf156
Compare
eaaf156 to
e29c05f
Compare
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.
OK yeah, I can support this. I did not want to put "additionalProperties": false in with "$ref" because that really is a big change, and some schemas out there (including the draft 4 hyper-schema meta-schema!) would suddenly become invalid.
I did not want to just exclude the validation properties without a lot of discussion and thought, because excluding validation keywords but not annotation or hyper-schema keywords means that we are implying that annotation and hyper-schema keywords are valid and usable in that position, and I think defining how that works is too complicated to sneak into a PR without a larger discussion. I am still very much open to having that larger discussion, preferably for the next draft :-)
While this technically makes some schemas invalid that were valid in draft 4 (since draft 4 did not validate "$ref" syntax at all), I seriously doubt anyone was relying on such schemas. So I can support this as a good tradeoff.
@handrews thank you.
Is there an issue to discuss restrictions on $ref?
Is there an issue to discuss restrictions on $ref?
No. You and awwright keep ignoring my requests to open one :-) (I haven't opened it because I'm not the one who wants to change it, although I am open to doing so).
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.
As far as I can tell, this isn't ever going to be incorrect. Looks good.
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.
Looks like it's merged. OK all the same.
Uh oh!
There was an error while loading. Please reload this page.
@handrews @awwright An alternative to #194, other changes seems to be merged already