-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Change HTTP DELETE status code and body logic, use nil for HTTP 204 #1774
base: master
Are you sure you want to change the base?
Conversation
definitely would need a good UPGRADING.md change to understand all that's changing and will take a look at the details in code, too. @namusyaka would appreciate another pair of eyes |
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.
lgtm with a nit.
spec/grape/dsl/inside_route_spec.rb
Outdated
it 'regards a nil as no content' do | ||
request = Grape::Request.new(Rack::MockRequest.env_for('/', method: 'DELETE')) | ||
subject.body nil | ||
expect(subject).to receive(:request).and_return(request) |
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 doesn't work because there is wrong code in using expect ~ receive
.
Depending on the implementation you modified, in this case the status
method is called internally.
As a result, the @status
variable is assigned and the request
member isn't called and it ends.
You don't need test this expectation. Please remove this line.
Sorry this took so long. I have processed the feedback and updated the CHANGELOG and UPGRADING files. I love to receive feedback again :) |
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.
Looking at the code this must affect more than DELETE
, no?
@@ -13,6 +13,7 @@ | |||
* [#1776](https://github.com/ruby-grape/grape/pull/1776): Validate response returned by the exception handler - [@darren987469](https://github.com/darren987469). | |||
* [#1787](https://github.com/ruby-grape/grape/pull/1787): Add documented but not implemented ability to `.insert` a middleware in the stack - [@michaellennox](https://github.com/michaellennox). | |||
* [#1788](https://github.com/ruby-grape/grape/pull/1788): Fix route requirements bug - [@darren987469](https://github.com/darren987469), [@darrellnash](https://github.com/darrellnash). | |||
* [#1774](https://github.com/ruby-grape/grape/pull/1774): Change HTTP delete status code and response body logic. Fix WEBrick compatibility - [@basjanssen](https://github.com/basjanssen). |
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.
Lets clarify this a little bit. How about this:
Returning an empty Hash or Array will no longer return 204 from HTTP DELETE
What's the story with WEBrick?
@@ -1,5 +1,9 @@ | |||
Upgrading Grape | |||
=============== | |||
### Upgrading to >= ? |
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.
Since this is a change in API I would make this 1.2, so bump that version part of this PR.
The section below becomes "Upgrading to >= 1.2".
The explanation can be clearer.
Instead of "Currently" say "Since version 1.2 returning ...".
There's also confusion here between body false
and body nil
. I would say "using body false
or body nil
everywhere.
Match the title to Returning an empty Hash or Array will no longer return 204 from HTTP DELETE.
Add a blank line after ;)
@body = '' | ||
status 204 | ||
def body(*value) | ||
raise ArgumentError 'you can only set a body with one argument' if value.length > 1 |
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.
I think the text is not required, just raise ArgumentError
.
What if value is nil
, what's nil.length
?
This needs a test.
You can also turn this into a separate PR if you want.
This PR has two goals:
Change HTTP DELETE status code and body logic
presenting {}. [], false will be regarded as content and will use HTTP status code 200. The content body will be presented with the set content.
Change using nil to set NO_CONTENT
Using nil will be more consistent with the way the content is regarded in the previous point.
Fixes #1768