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

use 500 as default status code for errors, instead of 400 #140

Merged
merged 1 commit into from
Apr 29, 2016

Conversation

lingyan
Copy link
Member

@lingyan lingyan commented Mar 31, 2016

400 is for client error. Propose to use 500 instead. Theoretically, this is a breaking change. We can make it a non breaking change, by allowing people to configure default error code to return. But don't think it is worth the trouble. We can go 1.0 with this :)

@Vijar @mridgway @redonkulus

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.796% when pulling db3b2fa on defaultErrorStatusCode into c530b0d on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.796% when pulling 41f145d on defaultErrorStatusCode into c530b0d on master.

@mridgway
Copy link
Collaborator

Weird. 👍

@mridgway
Copy link
Collaborator

I think there are other breaking changes that we would want to get in to 1.0. Is this actually a breaking change?

@lingyan
Copy link
Member Author

lingyan commented Mar 31, 2016

Yeah. Theoretically it is a breaking change, if some app monitors the error status code from fetcher :) This is not urgent though. We can wait for other breaking changes to publish 1.0.

@Vijar
Copy link
Contributor

Vijar commented Mar 31, 2016

👍

@yahoocla
Copy link

CLA is valid!

1 similar comment
@yahoocla
Copy link

CLA is valid!

@redonkulus
Copy link
Contributor

👍

@lingyan
Copy link
Member Author

lingyan commented Apr 29, 2016

Based on discussions, we will release this in a patch release, as a bug fix.

@lingyan lingyan merged commit 4886de2 into master Apr 29, 2016
@lingyan lingyan deleted the defaultErrorStatusCode branch April 29, 2016 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants