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.

Sign up for GitHub

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

Jump to bottom

No headers bodytype #56

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

No headers bodytype #56

wants to merge 3 commits into from

Conversation

JackKelly-Bellroy
Copy link

Stop BodyTypes incorrectly adding NoContent to the list of body types if it appears within a Headers (on endpoints that set response headers but don't return a response body).

@JackKelly-Bellroy
Whitespace lint
9aaeaf2
@JackKelly-Bellroy
Stop NoContent appearing in BodyTypes if it appears inside Headers
76e9429 
Recurse through `Headers hdrs` just like any other decorator that
should be ignored.
@JackKelly-Bellroy
Fix superclass conditions on HasOpenApi (UVerb method cs (a ': as))
1532c22 
Unconditionally requesting `ToSchema a` etc means that we erroneously
demand `ToSchema NoContent`. Instead delegate to the `HasOpenApi`
instance for the related `Verb`, allowing the overlapping instances
for `NoContent` to be selected.
@JackKelly-Bellroy
Copy link
Author

servant-openapi3 still asks for ToSchema NoContent if you have a WithStatus s NoContent in your API, but that feels dubious anyway. If we want to fix it, I think we should invert the chaining of instances currently used, where the instance for Verb method status cs a delegates to Verb method status cs (Headers '[] a). If we always peel off layers when we chain (e.g., a new instance HasOpenApi (Verb method status' cs a) => HasOpenApi (Verb method status cs (WithStatus status' a)) , we should be able to avoid having to add special cases for NoContent in multiple places, and have a single overlapping instance to catch Verb method status cs NoContent.

I wanted to check in before doing this (which can happen in a separate PR) in case the current instance layout was for a reason I wasn't aware of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@JackKelly-Bellroy