-
Notifications
You must be signed in to change notification settings - Fork 9.1k
v3.2: Clarify JSON-compatible YAML #4758
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
base: v3.2-dev
Are you sure you want to change the base?
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.
Looks good. 👍
src/oas.md
Outdated
Previous versions of this specification expressed this requirement in terms of [YAML's JSON schema ruleset](https://yaml.org/spec/1.2/spec.html#id2803231), which defines a subset of the YAML syntax and is unrelated to [[JSON-Schema-2020-12|JSON Schema]]. | ||
Despite its name, this ruleset supports features that are not part of JSON, such as distinguishing between integers and floats (see [Data Types](#data-types)) or supporting `!!float .nan` as a value. |
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 don't know that this is necessary. This content seems fine as part of the description in the PR, in case some one wants to understand the change later, but I don't think we need it in the spec. Just my 2 cents.
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 be OK with taking it out.
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 only thing that's really happening here is that the first sentence acknowledges why there may be a compatibility weirdness where something previously within the "RECOMMENDED" is now "NOT RECOMMENDED", but maybe we don't need to make as much of a point about that? The second sentence here is really just me over-explaining.
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.
FWIW, I think better to put it in the spec than force someone to track it down in the PR. (I think we need a better compromise in general, like footnotes.)
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.
@hudlow let me see if I can just word it better, so it feels less like a tangent.
After further discussions with @hudlow the character set stuff may not be needed regarding the Example Object, so it's worth considering whether that part needs to be here at all, or whether what's in the JSON and YAML RFCs is sufficient (it probably is). |
We don't really need the stuff about character encodings, as it was there because I was confused about something else. Also minimize the explanation of the change.
@hudlow @mikekistler I have removed the bit about character encodings (it is truly not needed, after getting a night of sleep- I've been under the weather, please forgive the confusion) and have slimmed down the explanation as much as I can while still conveying that there's a bit of a danger zone that was technically previously recommended, but was never really intended to work. |
I'm stuck at home bored and getting over a cold, so have a PR as I distract myself from missing holiday plans!
This fixes:
Everything in this area is a RECOMMENDED (== SHOULD) so we have some leeway. I have tightened what is recommended but acknowledged previous guidance and simply said that depending on anything allowed by the old guidance but not by the new is NOT RECOMMENDED. I suspect most tools never even noticed, TBH.