Change HTTP DELETE status code and body logic, use nil for HTTP 204#1774
Change HTTP DELETE status code and body logic, use nil for HTTP 204#1774basjanssen wants to merge 5 commits intoruby-grape:masterfrom
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 |
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.
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 :) |
dblock
left a comment
dblock
left a comment
There was a problem hiding this comment.
Looking at the code this must affect more than DELETE, no?
| * [#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.
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.
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.
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.
3 participants
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