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

3.2.0: minor nits #4026

Merged
merged 4 commits into from
Sep 7, 2024
Merged

3.2.0: minor nits #4026

merged 4 commits into from
Sep 7, 2024

Conversation

ralfhandl
Copy link
Contributor

@ralfhandl ralfhandl added the approved pr port PRs that just port an approved PR to another version label Aug 15, 2024
@ralfhandl ralfhandl requested review from a team as code owners August 15, 2024 13:12
@ralfhandl ralfhandl changed the base branch from main to v3.2.0-dev August 15, 2024 13:13
@ralfhandl ralfhandl marked this pull request as draft August 15, 2024 13:15
@ralfhandl ralfhandl added this to the v3.2.0 milestone Aug 16, 2024
Copy link
Contributor

@miqui miqui 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 to me so far.

Copy link
Member

@handrews handrews left a comment

Choose a reason for hiding this comment

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

I haven't looked over this or its predecessors thoroughly, but one thing jumped out. I'm happy to backport it since the 3.0.4/3.1.1 PRs were already merged. I haven't had time to figure out if there are other places in this change with the same concern.

Comment on lines +138 to 140
* Detecting a document containing a referenceable object at its root based on the expected type of the reference
* Allowing users to configure the type of documents that might be loaded due to a reference to a non-root object

Copy link
Member

Choose a reason for hiding this comment

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

I missed this in the changes on the other branches. I capitalized Object here very intentionally because it refers only to named capital-O Object types in the OAS, not to generic JSON objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about using "OAS-defined Object" here? That would be more noticeable than a single upper-case letter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix this in a follow-up sync PR, this is the direct port of #4014.

We have made changes to 3.1.1 in the meantime that need syncing anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@handrews There are more changes in

that need to be ported to 3.2.0, for example the definition of "Object" and adjustment of headlines.

If we want to collapse all diffs into one large PR, we can do that by closing this PR and enhancing

Which way to go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@handrews handrews left a comment

Choose a reason for hiding this comment

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

@ralfhandl can you re-do this with the tables not expanded since we agreed on that for the other branches? That will keep the change history so much less polluted on this branch.

Ideally I'd prefer not collapsing the spaces after periods, but I'm not going to my heels in on it (but it does create more noise and makes it harder to find sentence changes in a fixed-width editor).

Also please fix the two places where "Object" should be capitalized- I can also get it when porting the addition of the "Object" definition, but it would be better to not add change history noise there either if we can avoid it.

Copy link
Member

@handrews handrews left a comment

Choose a reason for hiding this comment

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

[Same comment as before, was supposed to be a "Request Changes" not "Approve", my apologies for hitting the wrong button]

@ralfhandl can you re-do this with the tables not expanded since we agreed on that for the other branches? That will keep the change history so much less polluted on this branch.

Ideally I'd prefer not collapsing the spaces after periods, but I'm not going to my heels in on it (but it does create more noise and makes it harder to find sentence changes in a fixed-width editor).

Also please fix the two places where "Object" should be capitalized- I can also get it when porting the addition of the "Object" definition, but it would be better to not add change history noise there either if we can avoid it.

@ralfhandl
Copy link
Contributor Author

Can do that after #4025 is merged as this builds on it.

@handrews
Copy link
Member

@ralfhandl I've approved the previous PR in the sequence, so once you merge and rebase I'll look over this one.

@ralfhandl ralfhandl marked this pull request as ready for review August 29, 2024 21:55
@ralfhandl ralfhandl requested review from miqui, handrews, mikekistler and a team August 29, 2024 21:55
@ralfhandl
Copy link
Contributor Author

@handrews & @miqui

once you merge and rebase I'll look over this one

Merged #4025 and rebased this one, please review

Copy link
Contributor

@miqui miqui left a comment

Choose a reason for hiding this comment

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

@ralfhandl checkout my comment

@@ -314,7 +314,7 @@ This is the root object of the [OpenAPI document](#openapi-document).
| <a name="oas-paths"></a>paths | [Paths Object](#paths-object) | The available paths and operations for the API. |
| <a name="oas-webhooks"></a>webhooks | Map[`string`, [Path Item Object](#path-item-object) \| [Reference Object](#reference-object)] ] | The incoming webhooks that MAY be received as part of this API and that the API consumer MAY choose to implement. Closely related to the `callbacks` feature, this section describes requests initiated other than by an API call, for example by an out of band registration. The key name is a unique string to refer to each webhook, while the (optionally referenced) Path Item Object describes a request that may be initiated by the API provider and the expected responses. An [example](../examples/v3.1/webhook-example.yaml) is available. |
| <a name="oas-components"></a>components | [Components Object](#components-object) | An element to hold various schemas for the document. |
| <a name="oas-security"></a>security | [[Security Requirement Object](#security-requirement-object)] | A declaration of which security mechanisms can be used across the API. The list of values includes alternative security requirement objects that can be used. Only one of the security requirement objects need to be satisfied to authorize a request. Individual operations can override this definition. To make security optional, an empty security requirement (`{}`) can be included in the array. |
| <a name="oas-security"></a>security | [[Security Requirement Object](#security-requirement-object)] | A declaration of which security mechanisms can be used across the API. The list of values includes alternative Security Requirement Objects that can be used. Only one of the Security Requirement Objects need to be satisfied to authorize a request. Individual operations can override this definition. To make security optional, an empty security requirement (`{}`) can be included in the array. |
Copy link
Contributor

Choose a reason for hiding this comment

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

To make security optional
Should be: To make security explicitly optional

To conform to https://github.com/OAI/OpenAPI-Specification/pull/4059/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@miqui I'd rather port this separately, similar to the 3.1.1 port in

Once 3.1.1 is published I'll do another diff between 3.1.1 and 3.2.0 to catch all accumulated differences.

Would that work for you?

Plus, this PR is the basis that the next PR port

is based on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ralfhandl ralfhandl merged commit f6316fc into OAI:v3.2.0-dev Sep 7, 2024
1 check passed
@ralfhandl ralfhandl deleted the 3.2.0-minor-nits branch September 7, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved pr port PRs that just port an approved PR to another version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants