-
-
Notifications
You must be signed in to change notification settings - Fork 357
Various language improvements #236
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
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.
Nice work!
While I still think we need to sort out a lot of things about what "default" really means, I agree that "has the same behavior" is an improvement. And I don't want to delay Draft 06 for a more thorough change, so I'm in favor of this.
I have a couple of other minor comments, mostly around how schemas can now be booleans and not just objects.
jsonschema-validation.xml
Outdated
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.
What about
Most validation keywords only constrain values within a certain primitive type.
We've often talked about validation as a constraint system, and I think it feels more intuitive than talking about exclusion.
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.
The language is sort of tricky overall... revised
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.
Should we specify this as a true schema instead of an empty schema? Admittedly, the behavior is the same but true conveys the intent of permitting everything more clearly.
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.
I think so too
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.
"empty schema" is language that was used before (that this PR doesn't introduce), can we work on this in a new PR and call this one 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.
If empty schema vs true is the only concern, I endorse doing that in a follow-up PR. We should check for it in more places than just these lines, and that will be trivially easy to check up on on its own.
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.
Yeah, that's the thing, it's used quite a few times
jsonschema-validation.xml
Outdated
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.
I think we just want to say
Each element of the array MUST be a valid JSON Schema.
in order to allow boolean schemas as well.
jsonschema-validation.xml
Outdated
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.
Same thing:
Each element of the array MUST be a valid JSON Schema.
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.
I thought that's what we did at first and then it was changed back for some reason... idk. I'll do this though.
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.
I think maybe I changed it like that in a PR that I ended up dropping? The boolean schema thing went through a few variations before we settled on it. Whatever happened, I think just saying "valid JSON Schema" is the best option- we already define what that means in an obvious place (the core spec).
jsonschema-validation.xml
Outdated
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.
Same thing:
This keyword's value MUST be a valid JSON Schema.
Of course, {"not": true} doesn't make much sense, I think it should be syntactically 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.
Looks great! Let's let @Relequestual and @epoberezkin weigh in since they had somewhat different opinions of the "same behavior" part, but this looks good to me.
@epoberezkin , @Relequestual are y'all OK with this? I'd like to get these fixes merged if so. There is no behavior change so it doesn't need to sit open for weeks.
I need more time to review this PR. I feel in some places it's actually more confusing, and I have ideas how to fix that, but I can't do it right now.
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.
I find the phrasing "A missing key word...", does not make me think that the missing key word in question is THIS key word (as in, the key word in where the phrase is being used).
I would much rather prefer something like "Omitting this key word has the same effect as...". I feel the intent is far clearer.
If it's just me that feels this way, please say.
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.
That's exactly the word I was looking for, actually
@Relequestual How's it look now
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.
All good in my books! =D
As such... I'm merging!
No description provided.