-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
Clarify data model types and formats #4045
Merged
Merged
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
99b0fd8
Clarify data model types and formats
handrews c589651
Use older JSON spec consistently.
handrews 898949f
Improve table, make section for formats
handrews 2bc4e48
Merge branch 'v3.1.1-dev' into pr/4045
ralfhandl 4b8378a
Sync with registry.
handrews 1c8f451
Use "JSON data types" consistently where relevant.
handrews 0b94cf5
More tweaks about integers.
handrews 151f316
Clarifications from mikekistler on OAS data type vs JSON schema kwd/f…
handrews 8f626a5
Update versions/3.1.1.md
ralfhandl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
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.
Section 6.3 "Mathematical Integers" is correct here. There is no 6.1.1 in that draft of JSON Schema.
I am against "any number with a zero fractional part" as a description as it is ambiguous as to whether it refers to the value or the textual representation.
Again, this is a JSON Schema thing. JSON Schema's wording is what matters, and OpenAPI CANNOT change the requirement. The only thing we can or should do here is reference the appropriate section of 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.
Quite right that there is no 6.1.1 in that draft. Let me see if I can find what I was looking at. In any event, I don't believe section 6.3 "defines integers -- this is the problem I was trying to fix, so let's find a good solution to that.
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.
Here's the correct link:
https://datatracker.ietf.org/doc/html/draft-bhutton-json-schema-validation-00#autoid-11
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.
JSON Schema Validation defines
"integer"
in section 6.1.1There 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 updated my suggested change. @handrews please review.
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.
@mikekistler the point of this link is to explain why
1.0
is an integer. The core spec §6.3 does that. That is literally the entire purpose of that section.This text here already explains that the
type
keyword allows"integer"
. If you also want to link to thetype
keyword's section, we can do that on the wordtype
(although I'd rather not- we don't do that for any other mention of any other JSON Schema keyword AFAICT so it would be a confusing inconsistency).If you want to look up the details of
type
, it's obvious that you should look up its section in the validation spec. On the other hand, the mathematical nature of integers in JSON Schema is a recurring point of confusion, and it's less obvious where to resolve it. Hence linking to the section that explains it.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.
@handrews When I follow the link to section 6.3 I see this text:
I don't think there is anything here that "defines integers", and I don't know why I would conclude from this text that "1.0" is an 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.
@mikekistler OK let me think on this more. §6.1.1 of the validation spec still isn't what should go here.