-
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
Minor param serialization and wording fixes #4081
Conversation
versions/3.0.4.md
Outdated
@@ -1127,6 +1127,10 @@ In order to support common ways of serializing simple parameters, a set of `styl | |||
|
|||
See [Appendix E](#appendix-e-percent-encoding-and-form-media-types) for a discussion of percent-encoding, including when delimiters need to be percent-encoded and options for handling collisions with percent-encoded data. | |||
|
|||
Note that parameter data can contain characters used as delimiters by certain `style` and `explode` configurations. | |||
For example, when using `style: "simple"`, the arrays `["monospace", "serif", "Arial,Helvetica,sans-serif"]` and `["monospace", "serif", "Arial", "Helvetica", "sans-serif"]` would both be encoded as `"monospace,serif,Arial,Helvetica,sans-serif"`. |
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.
Style value "simple" explicitly references RFC6570, section 3.2.2 which has an example for encoding commas in parameter values as %2C
:
{keys} semi,%3B,dot,.,comma,%2C
{keys*} semi=%3B,dot=.,comma=%2C
This example and the explicitly referenced section 3.2.1 imply that this example is encoded as monospace,serif,Arial%2CHelvetica%2Csans-serif
.
There is no need and actually no lee-way for API providers to document something different.
I think this paragraph and the next should be removed from this PR.
We could (and probably should) extend the style examples and change one of the color values to contain a comma, for example "black,shiny"
.
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.
@ralfhandl oh good catch! I'm going to take this part ouot of this PR and go read up and see what makes sense to do. I'll submit another PR assuming we should still say something.
* Explicitly set `explode: false` in an example as the default with `style: form` is `explode: true`; the `explode: true` example was also left explicit to reduce confusion. * Tidy up overly conversational (e.g. "our document") language that I'd meant to revisit but forgot about. * Include the Header Object as one of the places where the `style` keyword is used (even if it is the simplest case) * Minor grammar fix. * Fix a missing space before an RFC reference.
@ralfhandl ready for re-review |
@jeremyfiel Are you still fine with this PR? It is now a subset of what you approved, and @handrews will follow up on the removed part. |
Good catch, Ralf. I didn't recognize that either. I'm good with the rest of the pr. |
Style guide conformance.
A few last (really! I hope...) changes from feedback on Slack today and a few things spotted while compiling release notes. If anything here is controversial beyond a slight back-and-forth, I'd rather drop it than bog down the patch releases at this stage.
style
delimiters in parameter values by explaining that we don't define how to handle them. (NOTE: I had language about this in here at some point and it got dropped somewhere along the line- this uses an example from @zlondrej who brought it up on Slack today)explode: false
in an example as the default withstyle: form
isexplode: true
; theexplode: true
example was also left explicit to reduce confusion. (also caught by @zlondrej)style
keyword is used (even if it is the simplest case)