Skip to content
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

More contentEncoding example improvements (from feedback) #4301

Merged
merged 2 commits into from
Jan 17, 2025

Conversation

handrews
Copy link
Member

This incorporates ideas from @mikekistler that did not make it into #4294. Then I'll cherry-pick both commits over to v3.2-dev

@handrews handrews added examples requests for more or better examples in the specification clarification requests to clarify, but not change, part of the spec labels Jan 15, 2025
@handrews handrews added this to the v3.1.2 milestone Jan 15, 2025
@handrews handrews requested review from a team as code owners January 15, 2025 20:21
mikekistler
mikekistler previously approved these changes Jan 15, 2025
Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

Thanks @handrews! I left one very minor suggestion.

src/oas.md Outdated Show resolved Hide resolved
Co-authored-by: Mike Kistler <mikekistler@microsoft.com>
Copy link
Contributor

@ralfhandl ralfhandl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use an inconsistent mix of single-quotes, double-quotes, backticks, and no decoration for field names and field values within YAML comments.

The style of the very first example looks best, so propagating its style to the other examples:

  • double-quotes around field names
  • no decoration of media type values

@@ -1800,16 +1801,19 @@ requestBody:
schema:
type: object
properties:
# default for a string without `contentEncoding` is `text/plain`
# default content type for a string without `contentEncoding`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# default content type for a string without `contentEncoding`
# default content type for a string without "contentEncoding"

Align with line 1749

Comment on lines +1810 to +1811
# default content type for a schema without `type`
# is `application/octet-stream`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# default content type for a schema without `type`
# is `application/octet-stream`
# default content type for a schema without "type"
# is application/octet-stream

Align with lines 1749-1750

Comment on lines +1815 to +1816
# in the `items` subschema, which is an object here,
# so the default content type for each item is `application/json`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# in the `items` subschema, which is an object here,
# so the default content type for each item is `application/json`
# in the "items" subschema, which is an object here,
# so the default content type for each item is application/json

@@ -1833,7 +1837,7 @@ requestBody:
type: string
format: uuid

# Encoding Object overrides the default `application/json`
# Encoding Object overrides the default `application/json` content type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Encoding Object overrides the default `application/json` content type
# Encoding Object overrides the default application/json content type

@ralfhandl ralfhandl requested a review from a team January 16, 2025 16:35
@handrews
Copy link
Member Author

@ralfhandl we use backtics in YAML comments in many places. I would like to leave these as-is and then fix them throughout the document in a separate PR. There are some stylistic things to consider about the right way to handle this, including nested quotes of various sorts, so let's not introduce new variations without picking an overall style.

@handrews
Copy link
Member Author

@ralfhandl I have filed #4305 to track the further work.

Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. 👍

I am fine with deferring quoting fixes to a later PR.

@ralfhandl ralfhandl merged commit a922855 into OAI:v3.1-dev Jan 17, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification requests to clarify, but not change, part of the spec examples requests for more or better examples in the specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants