-
Notifications
You must be signed in to change notification settings - Fork 259
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: throw if response status is not int #715
Conversation
@ikitommi lmk if you want this somewhere else, or using another approach. I put the test in |
proposing @opqdonut as reviewer. |
@@ -125,7 +125,7 @@ | |||
(ring/router | |||
["/api" | |||
["/plus/:e" | |||
{:get {:responses {"200" {}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was actually using the wrong format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was actually a test case that was meant to throw an error due to reitit.ring.spec/validate. Now it throws your new error because it seems the response coercion compilation happens before the :validate
.
I propose you remove your latest commit and change this test case so that it fails in :validate
, for example:
{:get {:responses {200 {:description 1}}
:handler identity}}
(edit: fixed the suggestion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used your suggestion.
But it does beg the question, am I fixing the wrong thing? If reitit.ring.spec/validate
already caught the non-int response codes, maybe that's what I should be using in the first place. But then why is the validation optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question re: validation being optional. I think this added assert is a good addition anyway, though you are right that it is a bit redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix looks good, but the test needs work.
@@ -125,7 +125,7 @@ | |||
(ring/router | |||
["/api" | |||
["/plus/:e" | |||
{:get {:responses {"200" {}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was actually a test case that was meant to throw an error due to reitit.ring.spec/validate. Now it throws your new error because it seems the response coercion compilation happens before the :validate
.
I propose you remove your latest commit and change this test case so that it fails in :validate
, for example:
{:get {:responses {200 {:description 1}}
:handler identity}}
(edit: fixed the suggestion)
328e4f0
to
f0fc440
Compare
Thanks, all! |
Fix #667