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

Add compliant example sets to show expected behaviour #99

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lornajane
Copy link
Contributor

These examples are taken from the test suite of https://github.com/lornajane/openapi-overlays-js (and with thanks to @MikeRalphson who helped with the tests originally!) to illustrate the expected outputs from applying particular Overlays to OpenAPI descriptions.

I think these examples could be improved/expanded, and I'm not sure if we should try to somehow standardise how whitespace/quotes work because they are most of the diff. Would it make sense to lint/format all the yaml? Advice welcome!

@ralfhandl
Copy link
Contributor

we should try to somehow standardise how whitespace/quotes work because they are most of the diff

I'd run Prettier on the YAML files to minimize these differences.

@ralfhandl
Copy link
Contributor

There should also be JSON inputs.

And some "mixed" inputs with OpenAPI JSON and Overlay YAML, and vice versa. Would the output format be determined by the format of the OpenAPI input? The spec doesn't tell.

@lornajane
Copy link
Contributor Author

Fair enough. I don't think these are of a quality that we can publish for tests but from our conversation yesterday I thought we agreed that sharing whatever we had would be useful at this stage. Should I just close?

@ralfhandl
Copy link
Contributor

Don't close, I think these are a good starting point. Even if they aren't complete yet, they are behavior that we expect from each implementation.

@lornajane lornajane mentioned this pull request Nov 26, 2024
actions:
- target: $.paths['/pets'].get.summary
remove: true
- target: $.paths.*.get
Copy link

@ahx ahx Feb 27, 2025

Choose a reason for hiding this comment

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

Hey @lornajane, I have a question about this and the related output.yaml:

The spec says:

If the target selects an object node, the value of this field MUST be an object with the properties and values to merge with the selected node. If the target selects an array, the value of this field MUST be an entry to append to the array.

The related part of the output expects "wildcard-done" to be added to the list of tags. But the target ($.paths.*.get) does not select an array. So should this be $.paths.*.get.tags instead and the tags field removed from update?

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 think the point here is that the target of the expression is an object (the path item object) and I'm amending a property of it. Does that help?

Copy link

@ahx ahx Mar 11, 2025

Choose a reason for hiding this comment

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

Sorry, I should probably just ask somewhere else about how to read the spec. The spec says "merge", which I understand like Object.assign(a, b) in JavaScript, where if properties defined in a will be overwritten by the same properties defined b. Array values would just be overwritten and not concatenanted as in the related output's "tags" array.

Is my understanding wrong?

@ahx
Copy link

ahx commented Feb 27, 2025

This change is super useful, even in it's current state. I am building a ruby library against this right now ♥️

@lornajane
Copy link
Contributor Author

@ahx thanks for the encouraging words! I've updated the PR and re-requested reviews for it.

@lornajane lornajane requested review from ralfhandl and a team March 11, 2025 15:41
@mikeschinkel
Copy link

This looks really great and would really like to see it approved.

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.

Can we normalize all YAML files so that there are no formatting variations (single/double/no quotes, empty lines, ...) and the diff between openapi.yaml and output.yaml is just the changes made by overlay.yaml?

@lornajane lornajane requested review from ralfhandl and a team March 25, 2025 14:28
@lornajane
Copy link
Contributor Author

@ralfhandl Could you check if these were the changes you had in mind?

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.

Thanks, now I can just diff openapi.yamp and output.yaml and check whether it matches my expectation from overlay.yaml!

And they all do 😎

@ralfhandl ralfhandl requested a review from a team March 26, 2025 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants