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

Return more useful errors #4

Open
cbetta opened this issue Oct 7, 2016 · 6 comments
Open

Return more useful errors #4

cbetta opened this issue Oct 7, 2016 · 6 comments

Comments

@cbetta
Copy link
Contributor

cbetta commented Oct 7, 2016

In the ruby library I've tried to catch most errors and bubble them up in a meaningful way. For example when I send a fax to a number that I'm not yet allowed to send a fax to I get:

fax = interfax.deliver(faxNumber: "+11111111112", file: 'spec/test.pdf')

InterFAX::Client::BadRequestError: Bad request (400): {"code":-111,"message":"Attempting to fax to a number that is not the designated fax number in a developer account","moreInfo":null}

In this library I get

>>> fax = interfax.deliver(fax_number="+442084978650", files=["tests/test.pdf"])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "interfax/outbound.py", line 20, in deliver
    files=self._generate_files(files))
  File "interfax/client.py", line 76, in post
    return self._request('POST', url, **kwargs)
  File "interfax/client.py", line 89, in _request
    return self._parse_response(request(method, url, **kwargs))
  File "interfax/client.py", line 117, in _parse_response
    response.raise_for_status()
  File "/Users/cbetta/.pyenv/versions/2.7.12/lib/python2.7/site-packages/requests/models.py", line 862, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 400 Client Error: Bad Request for url: https://rest.interfax.net/outbound/faxes?faxNumber=%2B442084978650
@cbetta
Copy link
Contributor Author

cbetta commented Oct 7, 2016

FYI here is the pretty basic logic I have applied in the Ruby gem https://github.com/interfax/interfax-ruby/blob/ccefaf015320403d9ff5eb37bc259a7686500b3f/lib/interfax/client.rb#L116-L128

@danielknell
Copy link
Contributor

I did that initially but that error class has additional info that could be useful when debugging, and seemed more sensible than copying all that data across into a new hierarchy of errors, open to changing it though.

@cbetta
Copy link
Contributor Author

cbetta commented Oct 10, 2016

@danielknell oh I agree on that. I'm not too worried about wrapping it in new error objects, that's just a convenience. What I am more worried about is that my error exposes the JSON body:

{"code":-111,"message":"Attempting to fax to a number that is not the designated fax number in a developer account","moreInfo":null}

Which is actually useful to the end user. Your code just tells me Bad Request without context.

@cbetta
Copy link
Contributor Author

cbetta commented Feb 1, 2017

@danielknell where did we leave this?

@danielknell
Copy link
Contributor

danielknell commented Feb 2, 2017

I got weighed down with other stuff and then forgot about it, sorry, i will try and find some time to look soon.

@cbetta
Copy link
Contributor Author

cbetta commented Feb 2, 2017

Thanks @danielknell, any time you might have available to make this change would be much appreciated.

@danielknell danielknell removed their assignment Mar 12, 2021
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

No branches or pull requests

2 participants