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

Fixed client error status code range. #735

Closed
wants to merge 1 commit into from

Conversation

borsothy
Copy link
Contributor

@borsothy borsothy commented Oct 2, 2017

No description provided.

@borsothy
Copy link
Contributor Author

borsothy commented Oct 2, 2017

I understand the idea behind raising the same exception type for 4xx and 5xx responses, but the naming is really confusing. 5xx are server errors, and it makes it really hard to filter for client and server errors separately.

This is more of a proposal than a real solution, tests are failing, and obviously more work needs to be done - please let me know what you think, and how this should be addressed.

@iMacTia
Copy link
Member

iMacTia commented Oct 5, 2017

Hi @borsothy, I totally agree with you, I think here we should have a Faraday::Error::ClientError (4xx), but also a Faraday::Error::ServerError (5xx).
The change itself looks quite easy, but I'm really scared about backward compatibility.
Unless we can think of a backward compatible fix, I can only push this into v1.0

@iMacTia iMacTia added this to the v1.0 milestone Oct 5, 2017
@iMacTia
Copy link
Member

iMacTia commented Nov 12, 2017

Hi @borsothy,
I've added this to the v1.0 list and changed the base branch to v1.0.
The fix itself is ok, but as I said we need to introduce a new Faraday::Error::ServerError class that covers errors 5xx and tests will obviously need some work as well.

Would you still like to work on this?

@bronislav
Copy link
Contributor

As an option, we may make this configurable and allow users to explicitly agree with correct statuses and let others use old ones.

@borsothy, @iMacTia what do you think?

@iMacTia
Copy link
Member

iMacTia commented Feb 28, 2018

@bronislav I'd rather take advantage of v1.0 (backwards incompatible) to "force" everyone following the correct convention. The configuration would have been a good solution if we wanted to introduce this in 0.x versions as well

@iMacTia iMacTia mentioned this pull request Jan 4, 2019
4 tasks
@iMacTia
Copy link
Member

iMacTia commented Jan 4, 2019

Closing in favour of #841

@iMacTia iMacTia closed this Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants