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 test for OPTIONS and HEAD requests with catch-all #1326

Merged
merged 2 commits into from
Mar 16, 2016

Conversation

namusyaka
Copy link
Contributor

ref #1057
I've cherry-picked @ekampp's commit and fixed a little bit.
Thoughts?

ekampp and others added 2 commits March 17, 2016 01:06
This illustrates the issue described in ruby-grape#1056 by adding a options and
head request when the catch-all route is enabled.
@ekampp
Copy link
Contributor

ekampp commented Mar 16, 2016

@namusyaka
Copy link
Contributor Author

Well, OPTIONS seems not to have the restriction of status codes.
Our default OPTIONS routes are defined at this line, and this means that a response of the route will be empty.
So I guess 204 status is reasonable, right?

@ekampp
Copy link
Contributor

ekampp commented Mar 16, 2016

204 is reasonable for an empty response. But I'm reading that it should return 200 OK. But either way getting this to pass would be an improvement 👍

@dblock
Copy link
Member

dblock commented Mar 16, 2016

Very good, the passing spec is a merge. Lets close the related bug and review whether we want to change the status code here (open new issues as appropriate).

dblock added a commit that referenced this pull request Mar 16, 2016
Add test for OPTIONS and HEAD requests with catch-all
@dblock dblock merged commit f5d3ddc into ruby-grape:master Mar 16, 2016
@dblock
Copy link
Member

dblock commented Mar 16, 2016

This possibly needs a CHANGELOG entry since I believe this was a known bug now fixed, @namusyaka please.

namusyaka added a commit to namusyaka/grape that referenced this pull request Mar 16, 2016
dblock added a commit that referenced this pull request Mar 16, 2016
@namusyaka namusyaka deleted the fix-1057 branch March 22, 2016 12:26
przbadu pushed a commit to przbadu/grape that referenced this pull request Apr 5, 2016
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.

3 participants