Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Specify "format" requirements in vocabulary terms #764
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
Specify "format" requirements in vocabulary terms #764
Changes from all commits
3f54ba2
3ea9e82
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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've never noticed this before, but it sounds not what you want...
Say you have a format "smallint". If a string is provided at the applicable instance, validation will assert TRUE if the format says it is only applicable to numeric types.
I guess this makes sense, because it's similar to how other keywords work, in that keywords are often only applicable if they target an instance of the same type.
For example,
maxLength: 5
fails validation for a string of longer than 5 characters, but does not fail validation for a numeric value123456789
because it is not the correct applicable type.This is covered explicitly in draft-8 core (http://json-schema.org/work-in-progress/WIP-jsonschema-core.html#rfc.section.7.4.1)
But, I feel we should maybe re-reference this section here for clarity...
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'd rather just take the "SHOULD succeed" language out entirely. We used to have that phrasing everywhere and this is just left over from that. As you note, this is typical behavior, so it's better not to call it out- calling things out should be done for atypical behavior.
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.
Agreed.
Is there some text about "if a keyword isn't applicable to the types it may be applied to, then it is not applied and therefore is the same as a true assertion" generally?
Take
minimum
:The intent is clear, but the phrasing and wording is... not very spec like.
We don't explicitly specify what happens if the instance is not a number...
The behaviour is, in essense, undefined (unless I've missed something).
If that's the case... maybe a spec cleanup is a candidate for draft-9.
Sorry, I've got way off topic here... but it's relevant.
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.
"We" should build some tests around collecting format values as annotations.
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. And really around annotation collection in general.
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.
Do we have an issue to track this?
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.
@philsturgeon no, but feel free to file one 😄