-
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
3.0.4: minor nits #4001
3.0.4: minor nits #4001
Conversation
and two other uses of it in this document
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 of these changes are helpful!
(oh wait it's a draft... well, I'll review it again when you're done :-) |
@@ -2409,22 +2411,22 @@ This mechanism is used by [Link Objects](#link-object) and [Callback Objects](#c | |||
The runtime expression is defined by the following [ABNF](https://tools.ietf.org/html/rfc5234) syntax | |||
|
|||
```abnf | |||
expression = ( "$url" / "$method" / "$statusCode" / "$request." source / "$response." source ) | |||
source = ( header-reference / query-reference / path-reference / body-reference ) | |||
expression = "$url" / "$method" / "$statusCode" / "$request." source / "$response." source |
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.
Are we fine with the fact that ABNF literal text strings enclosed in double-quotes are case-insensitive?
For example "$url"
matches values $uRl
, $url
, $URL
and any other case variation.
Do we assume this to be general knowledge, or should we call it out?
@ralfhandl could you include this clarification? |
@@ -2682,7 +2684,7 @@ Other than the JSON Schema subset fields, the following fields MAY be used for f | |||
|
|||
| Field Name | Type | Description | | |||
| ----------------------------------------------- | :-----------------------------------------------------------: | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | |||
| <a name="schema-nullable"></a>nullable | `boolean` | A `true` value adds `"null"` to the allowed type specified by the `type` keyword, only if `type` is explicitly defined within the same Schema Object. Other Schema Object constraints retain their defined behavior, and therefore may disallow the use of `null` as a value. A `false` value leaves the specified or default `type` unmodified. The default value is `false`. | |
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 old wording seems to mean
Mentally put the string value of
type
into an array and add the string"null"
to this array, then interpret the array in the JSON Schema sense because OpenAPI explicitly forbids array values for thetype
keyword in the preceding section.
The new text hopefully avoids this mental gymnastics and the possible confusion of null
as an allowed value and "null"
as an additional string item in the array value of the JSON Schema type
keyword.
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.
Not only are array values for type forbidden, but null
is also forbidden.
null is not supported as a type
But I'm not sure the new wording is better. In particular, the "allowed values" reference could be confused with enum
. Oh, but actually we should add null
to the enum if it exists. JSON Schema says:
Elements in the array MAY be of any type, including null.
An instance validates successfully against this keyword if its value is equal to one of the elements in this keyword's array value.
Oh my.
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 agree with @mikekistler - I find the new wording more confusing. I would go with something like:
This keyword only takes effect if
type
is explicitly defined within the same Schema Object. Atrue
value indicates that bothnull
values and values of the type specified bytype
are allowed. Other Schema Object constraints retain their defined behavior, and therefore may disallow the use ofnull
as a value. Afalse
value leaves the specified or defaulttype
unmodified. The default value isfalse
.
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.
Took over Henry's proposal
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. 👍
The `example` and `examples` fields are mutually exclusive, and if either is present it SHALL _override_ any `example` in the schema. | ||
|
||
Serializing with `schema` is NOT RECOMMENDED for `in: cookie` parameters, `in: header` parameters that use HTTP header parameters (name=value pairs following a `;`) in their values, or `in: header` parameters where values might have non-URL-safe characters; see [Appendix D](#appendix-d-serializing-headers-and-cookies) for details. | ||
Serializing with `schema` is NOT RECOMMENDED for `in: "cookie"` parameters, `in: "header"` parameters that use HTTP header parameters (name=value pairs following a `;`) in their values, or `in: "header"` parameters where values might have non-URL-safe characters; see [Appendix D](#appendix-d-serializing-headers-and-cookies) for details. |
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'm curious about the convention we are using for string values here and elsewhere. The original text has in: cookie", which in YAML is valid and I think is preferred over the new text
in: "cookie"`. The new text is not valid JSON. So what is our convention and why did we choose 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.
@mikekistler Please see the PR description:
- fixed fields are monospaced
This is the predominant style for fixed fields in the existing text.
- field values are monospaced in JSON notation:
true
,false
,null
,"header"
, ...
This is done for example in the first table in section https://spec.openapis.org/oas/v3.0.3.html#fixed-fields-9, and I find it helpful that field names
and field "values"
are clearly distinguished, so I applied this pattern wherever I wondered whether something is a field name or a field value.
The second table in the same section does not use that convention and is rather hard to read. For example
When
style
isform
, the default value istrue
.
Here form
is the content of a JSON string: is true
also the content of a JSON string, or is it a JSON boolean? How much context do I have to read to figure that out?
The pattern in: query
was introduced by Henry in 3.0.4, and I first tried to rephrase that into a style already present in 3.0.3:
if
in
is"query"
That felt clumsy in some places, so I opted for in: "query"
etc., combining conventions 5 and 6.
The PR description is my stab at an "editor's style guide", and of course open for discussion, and destined for documentation, for example in CONTRIBUTING.md
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.
Sorry I glossed over the PR description, and thank you for writing this!
On the particular style choice, I think I would slightly prefer YAML to JS, just to avoid adding another syntax style to the doc, but I can live with either as long as we are consistent.
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. 👍
Mostly harmonization of terminology and formatting:
monospaced
monospaced
true
,false
,null
,"header"
, ...in: "header"
, combining rules 5 and 6array
orobject
" - "type" is not monospaced, so the monospaced values aren't enclosed in double quotes, I find this easier to read