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

feat(dynamite): Support default responses #1051

Merged
merged 2 commits into from
Oct 29, 2023

Conversation

provokateurin
Copy link
Member

Closes #871

I had this laying around from the petstore example. Might as well merge it already now that we have tests for dynamite.

Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

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

How does this handle having both a status code and a default response specified?

I think we really have to make the status code validation taka a function that uses a switch case. This would also allow us to handle status code ranges.

@provokateurin
Copy link
Member Author

It is handled the same way as we currently do for multiple status codes: As long as they have the same schema they will be handled the same. If we have a default response then we can ignore if we have other status codes with the same schema because they will be handled by the default already.
I don't think we need your function for this yet (although we certainly need it in the future for supporting multiple schemas). I would like to keep everything as simple as possible until we actually need something more complex.

@provokateurin provokateurin force-pushed the feature/dynamite/default-responses branch from 419da46 to 9e34f0e Compare October 29, 2023 12:39
@Leptopoda
Copy link
Member

The please also add a test that handles this (having both a default and a status code)

@provokateurin provokateurin force-pushed the feature/dynamite/default-responses branch from 9e34f0e to 83c7db7 Compare October 29, 2023 13:46
Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

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

LGTM

@Leptopoda
Copy link
Member

You'll need to rebase as I just merged #1025.

@provokateurin provokateurin force-pushed the feature/dynamite/default-responses branch from 83c7db7 to ef7ff10 Compare October 29, 2023 15:58
@provokateurin provokateurin merged commit 1545426 into main Oct 29, 2023
9 checks passed
@provokateurin provokateurin deleted the feature/dynamite/default-responses branch October 29, 2023 16:08
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.

Support default responses
2 participants