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

sets 204 as default status for delete #1532

Merged
merged 1 commit into from
Dec 5, 2016
Merged

Conversation

LeFnord
Copy link
Member

@LeFnord LeFnord commented Dec 4, 2016

No description provided.

@LeFnord
Copy link
Member Author

LeFnord commented Dec 5, 2016

@namusyaka, @dblock … any objections, meanings?

@namusyaka
Copy link
Contributor

This change looks reasonable.
In fact, the reasonable status code that can occur in the normal route of the delete route would be 204, 202, and 200.
I think that 204 is the most frequently used among them.

@@ -5,7 +5,7 @@ Next Release

* [#1503](https://github.com/ruby-grape/grape/pull/1503): Allow to use regexp validator with arrays - [@akoltun](https://github.com/akoltun).
* [#1507](https://github.com/ruby-grape/grape/pull/1507): Add group attributes for parameter definitions - [@304](https://github.com/304).
* Your contribution here.
Copy link
Member

Choose a reason for hiding this comment

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

Put this back please.

@@ -41,6 +41,19 @@ Prior to this version the response would be `one is missing`.

See [#1510](https://github.com/ruby-grape/grape/pull/1510) for more information.

#### Change default status code of DELETE to 204

**Breaking change**: Sets the default response status code for a delete request to 204.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to highlight this in an extra way as a breaking change. We have never done this for other breaking changes.

**Breaking change**: Sets the default response status code for a delete request to 204.
Doing it makes the response more explicit, cause a delete request returns in most cases an empty body – the resource was deleted/voided. By setting it to 204, other as 200, makes the handling on consumer side easier.

To achieve the old behaviour, one has to set it explicitly:
Copy link
Member

Choose a reason for hiding this comment

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

Lets make this American, so behavior :)

#### Change default status code of DELETE to 204

**Breaking change**: Sets the default response status code for a delete request to 204.
Doing it makes the response more explicit, cause a delete request returns in most cases an empty body – the resource was deleted/voided. By setting it to 204, other as 200, makes the handling on consumer side easier.
Copy link
Member

Choose a reason for hiding this comment

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

How about A status of 204 makes the response more distinguishable and therefore easier to handle on the client side, particularly because a DELETE request typically returns an empty body as the resource was deleted or voided.

delete :id do
status 200
'foo successfully deleted'
end
Copy link
Member

Choose a reason for hiding this comment

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

Add a note like For more information see ... referencing this pull request.

@@ -41,6 +41,19 @@ Prior to this version the response would be `one is missing`.

See [#1510](https://github.com/ruby-grape/grape/pull/1510) for more information.

#### Change default status code of DELETE to 204
Copy link
Member

Choose a reason for hiding this comment

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

Should the change, so The default status code for DELETE is now 204 instead of 200.

@dblock
Copy link
Member

dblock commented Dec 5, 2016

I support this. Made some minor UPGRADING comments.

I think we should document this in README as well, along with the default status code being 200 for the other verbs.

@LeFnord
Copy link
Member Author

LeFnord commented Dec 5, 2016

@dblock … please can you provide a sample, how the changelog should looks like, so that danger accept it?

- adds changelog entry
- addresses comments
- next try to make danger happy ;)
@dblock
Copy link
Member

dblock commented Dec 5, 2016

Sounds like you got it, thanks!

@dblock
Copy link
Member

dblock commented Dec 5, 2016

I'm merging this.

@dblock dblock merged commit a3a28f5 into ruby-grape:master Dec 5, 2016
@connorshea connorshea mentioned this pull request Dec 15, 2016
@urkle
Copy link
Contributor

urkle commented Dec 23, 2016

This seems an odd change to make.. or, IMHO if the request did return a response it should switch back to 200.. (e.g. 204 should only be returned IF there really is no content).. As it is incorrect (according to the HTTP spec) to return ANY body when a 204 response is returned.

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.

4 participants