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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add exception for conflict response code #979

Merged

Conversation

lucasmoreno
Copy link
Contributor

Description

I wanted to rescue an exception for 409 response code and I noticed that it was not implemented yet. So, here it is 馃槃

@iMacTia
Copy link
Member

iMacTia commented May 16, 2019

Hi @lucasmoreno and thanks for working on this.
I'd just like to clarify that we didn't create a custom error class for every http status code because that would be just too much 馃槄, so we created them for Rails' most common ones and the one related to proxy authentication.
All other http status codes can be rescues with the generic ClientError and then you can access the status through the exception response property.

I personally don't have anything against this PR per-se, but I don't think that 409 is such a common http status to justify it's inclusion in the custom set of exceptions.

Would like to hear the opinion of the other mantainers though 馃槃

@lucasmoreno
Copy link
Contributor Author

Yeah, it makes sense! We should not have all the status codes mapped or the code would get huge 馃槃

I think 409 should have its specific exception because some APIs use it to indicate that some entity was already created and, in some scenarios, it can be understood as an indication that "the job was already done".

@iMacTia iMacTia merged commit 91ba32e into lostisland:master May 23, 2019
@iMacTia
Copy link
Member

iMacTia commented May 23, 2019

Discussed with the other maintainers offline and decided to give it the green light 馃憤.
Thanks for contributing!

@technoweenie technoweenie mentioned this pull request Jun 25, 2019
3 tasks
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.

None yet

2 participants