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

fix, test and document openapi named examples #639

Merged
merged 16 commits into from
Sep 11, 2023
Merged

Conversation

opqdonut
Copy link
Member

@opqdonut opqdonut commented Aug 28, 2023

leftovers from #627
related to #84

@opqdonut opqdonut force-pushed the fix-openapi-examples branch from 39b69e1 to 38547e4 Compare August 28, 2023 12:44
@opqdonut opqdonut changed the title fix, test and document openapi examples fix, test and document openapi named examples Aug 28, 2023
Copy link
Member

@ikitommi ikitommi 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, one weird thing I noticed.


["/pizza"
{:get {:summary "Fetch a pizza | Multiple content-types, multiple examples"
:content-types ["application/json" "application/edn"]
Copy link
Member

Choose a reason for hiding this comment

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

wait what? when and why did we add the :content-types? isn't the map keys in :content the correct source of supported types? I feel this is duplicate info which can can cause hard to track errors, e.g. defining inputs for multiple content-types under :content and not setting this value will cause only "application/json" to be supported (the default from code).

Copy link
Member

Choose a reason for hiding this comment

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

e.g. this is not part of OpenAPI Spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

:content-types is there as the analogue of :swagger {:produces [...]}. It's not directly copied to the openapi spec, but is used when expanding the :default content type. Openapi doesn't support a default case so we need to list out all the supported content types.

The list could be fetched from muuntaja when we do #636

That said, this is probably not needed here, so I'll remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, for the :post case it is needed since it uses a :default case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed one :content-types, added a comment for the other.

@opqdonut opqdonut requested a review from ikitommi September 8, 2023 12:18
Copy link
Member

@ikitommi ikitommi 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, one spec-error (we don't use the keys in tests I presume)

@opqdonut
Copy link
Member Author

opqdonut commented Sep 11, 2023

also fixed :tags spec & syntax

@opqdonut opqdonut merged commit 6360fa8 into master Sep 11, 2023
@opqdonut opqdonut deleted the fix-openapi-examples branch September 11, 2023 08:04
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.

2 participants