Added support for error handling by dibyadas · Pull Request #17 · sendgrid/python-http-client · GitHub

This PR deals with issue #16

Custom exceptions for python_http_client have been added. Only few exceptions have been added.
Others will be added soon.

All committers have signed the CLA.

Hi @thinkingserious ! I have added custom exception handling. Do suggest changes for improvements.
(P.S - Sorry for the delay. I got caught up in some work)

Hi @w- ! Have a look at this. What do you think? :)

Hey guys, i'll be out of town on vacation for the next few days. won't be able to respond on this topic till then.

Thanks for the heads up @w- and I hope you enjoy the vacation :)

@w-,

Do you have time to take a look? Thanks!

@thinkingserious,

In the meantime, how about I add tests for this?
Where do you think would be a good place to add?
Should I add it in the existing one or a different file for testing exceptions?

With Regards,
Dibya Prakash Das

@dibyadas,

Please add the test in the existing file. Thanks!

even though there are many classes., I would change this to an explicit import

Agreed.

this should probably get_HTTPError(error) and it should return an instance of the specific error class instead of raising it.

Umm.. Can you give an example? I can't visualize this.

this is what i think the function should be

def get_HTTPError(error):
	try:
		http_err_obj = err_dict[error.code](error)
	except KeyError:
		http_err_obj = HttpError(error)		
	return http_err_obj

we want a try/catch here in event that the error.code doesn't match anything in our dict. it should then set err as a generic HTTPError

Actually I just put in a few errors, the ones that SendGrid servers returns, just for showing the approach. If approved, I will add the rest of the error.

@dibyadas,

Thanks, that would be awesome. I also agree that we should make sure to try/catch for errors we don't expect as @w- illustrated.

I was pressing the wrong button earlier so my comments are split up.
Good work @dibyadas

as per https://www.python.org/dev/peps/pep-3134/
we probably want __cause__ to be the original error.

If we won't to do this then an ugly traceback is produced in Python 3. In this PR, we are raising our custom exception when an urllib.HTTPError is raised. Here, in python 3 , such a thing will be displayed the output.

Traceback (most recent call last):
  File "/home/dibya/workspace/python-http-client/python_http_client/client.py", line 141, in _make_request
    return opener.open(request)
  File "/usr/lib/python3.5/urllib/request.py", line 472, in open
    response = meth(req, response)
  File "/usr/lib/python3.5/urllib/request.py", line 582, in http_response
    'http', request, response, code, msg, hdrs)
  File "/usr/lib/python3.5/urllib/request.py", line 504, in error
    result = self._call_chain(*args)
  File "/usr/lib/python3.5/urllib/request.py", line 444, in _call_chain
    result = func(*args)
  File "/usr/lib/python3.5/urllib/request.py", line 696, in http_error_302
    return self.parent.open(new, timeout=req.timeout)
  File "/usr/lib/python3.5/urllib/request.py", line 472, in open
    response = meth(req, response)
  File "/usr/lib/python3.5/urllib/request.py", line 582, in http_response
    'http', request, response, code, msg, hdrs)
  File "/usr/lib/python3.5/urllib/request.py", line 510, in error
    return self._call_chain(*args)
  File "/usr/lib/python3.5/urllib/request.py", line 444, in _call_chain
    result = func(*args)
  File "/usr/lib/python3.5/urllib/request.py", line 590, in http_error_default
    raise HTTPError(req.full_url, code, msg, hdrs, fp)
urllib.error.HTTPError: HTTP Error 401: UNAUTHORIZED

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "test.py", line 7, in <module>
    r = a.name.api.get()
  File "/home/dibya/workspace/python-http-client/python_http_client/client.py", line 209, in http_request
    return Response(self._make_request(opener, request))
  File "/home/dibya/workspace/python-http-client/python_http_client/client.py", line 143, in _make_request
    handle_error(err)
  File "/home/dibya/workspace/python-http-client/python_http_client/exceptions.py", line 53, in handle_error
    raise exc
python_http_client.exceptions.UnauthorizedError: HTTP Error 401: UNAUTHORIZED

If we set the __cause__ to None, then only the part below During handling of the above exception, another exception occurred: is shown, which is all that should be displayed.

I see. ok. I guess it is preferable to be None

both headers and body are important to record in the new HTTPError object.

The HTTPError thrown by urllib module doesn't have a body attribute and in python versions <= 3.3, they don't have the headers attribute as well. I just commented this out so that this can be discussed further.

it does. you have to read docs and look at the source.

HTTPError is based on URLError which has the following attributes

err.url
err.code
err.hdrs
# to get the body
err.read()

Ah! Since it was not mentioned in the documentation, I missed it. Thanks for the tip! :)

keeping in line with other http libraries, I think that self.code should actually be self.status_code. personal preference only.

Sure we can. It'll be more evident.

@w- Here, I am handling the urllib.HTTPError. What you are saying is, we do this:-

try:
    return opener.open(request)
except HTTPError as err:
    raise get_HTTPError(err)

Am I right?

correct.

Nice work @dibyadas and @w-! Looks like we are almost there :)

Hi @thinkingserious and @w- ,
I am not that familiar with adding tests, but I have tried to add some. If have also tried addressing your review comments. Please have a look and suggest changes if any.

Edit :- After pushing an commit the coverage passed but is now failing 😕 I not really familiar with this. I need some help here.

With Best Regards,
Dibya Prakash Das

Hi @thinkingserious,
I thought it would be good, if you could take a look at this. I am kind of stuck here. The code coverage passed and then failed for reasons I do not know. I thought maybe you could help me with this.

With Regards,
Dibya Prakash Das

This looks good to me @dibyadas, what do you think @w-?

I'm ok with this.

Hello @dibyadas,

Thanks again for the PR!

We want to show our appreciation by sending you some swag. Could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

Hi @thinkingserious,
I have filled up the from. Thanks a lot!
With Regards,
Dibya Prakash Das

Per sendgrid/sendgrid-python#323 if we could get examples of these awesome new exceptions in the README so other devs don't need to go surfing into tests to see examples of exceptions and what can cause them, I think that'd be super helpful.

Please check code using flake8/pep8

could you please use spaces and pep8

thinkingserious added the status: code review request requesting a community code review or review from Twilio label May 24, 2017

w- reviewed Jun 6, 2017

View reviewed changes

dibyadas commented Jun 6, 2017

View reviewed changes

SendGrid's DX Team and others added 2 commits June 9, 2017 21:06

Update CLA process

08f4249

add support for error handling

86b2f08

dibyadas force-pushed the error_handling branch from ff3e16d to 86b2f08 Compare June 9, 2017 15:46

dibyadas added 3 commits June 9, 2017 21:38

few fixes

dfa222a

fix

964c0f8

fix

a89c59a

dibyadas force-pushed the error_handling branch from 4bf0630 to a89c59a Compare June 9, 2017 16:27

trigger travis

2a3b176

thinkingserious added 4 commits June 16, 2017 14:23

Updating coveralls init

5019bf0

Updating coveralls init

76f7d8d

Updating coveralls init

d40c43d

Remove coveralls

2a87bd5

sendgrid deleted a comment from coveralls Jun 16, 2017

thinkingserious merged commit 1c71517 into sendgrid:master Jun 21, 2017

thinkingserious added a commit that referenced this pull request Jun 21, 2017

Version Bump v2.3.0: #17 Added support for error handling

45726d7

thinkingserious mentioned this pull request Sep 15, 2017

Mail Helper Refactor - Call for Feedback sendgrid/sendgrid-python#323

Closed

andriisoldatenko reviewed Sep 15, 2017

View reviewed changes

thinkingserious mentioned this pull request Sep 27, 2017

Can BadRequestError be made more intelligent / user friendly? sendgrid/sendgrid-python#346

Closed

thinkingserious mentioned this pull request Oct 26, 2017

Document the new error handling functionality from python-http-client sendgrid/sendgrid-python#442

Closed

+              	def __init__(self,error):
+              		self.code = error.code
+              		self.reason = error.reason
+              		#self.headers = error.headers

+              class HTTPError(Exception):
+              	''' Base of all other errors'''
+              	def __init__(self,error):
+              		self.code = error.code

		@@ -1 +1,2 @@
		from .client import Client
		from .exceptions import * No newline at end of file

+: ServiceUnavailableError
+              }
+              def handle_error(error):

+              def handle_error(error):
+              	exc = err_dict[error.code](error)
+              	exc.__cause__ = None

+              	pass
+              class MethodNotAllowedError(HTTPError):
+              	pass

Added support for error handling by dibyadas · Pull Request #17 · sendgrid/python-http-client · GitHub

Uh oh!

Added support for error handling - #17

Added support for error handling#17thinkingserious merged 10 commits intosendgrid:mastersendgrid/python-http-client:masterfrom dibyadas:error_handlingdibyadas/python-http-client:error_handlingCopy head branch name to clipboard

Conversation

dibyadas commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SendGridDX commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dibyadas commented May 24, 2017

Uh oh!

dibyadas commented May 24, 2017

Uh oh!

w- commented May 24, 2017

Uh oh!

thinkingserious commented May 26, 2017

Uh oh!

thinkingserious commented Jun 1, 2017

Uh oh!

dibyadas commented Jun 2, 2017

Uh oh!

thinkingserious commented Jun 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w- Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w- left a comment

Choose a reason for hiding this comment

Uh oh!

w- Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w- Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thinkingserious commented Jun 7, 2017

Uh oh!

dibyadas commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dibyadas commented Jun 16, 2017

Uh oh!

thinkingserious commented Jun 16, 2017

Added support for error handling#17
thinkingserious merged 10 commits into
sendgrid:masterfrom
dibyadas:error_handling

dibyadas commented May 24, 2017 •
edited
Loading

SendGridDX commented May 24, 2017 •
edited
Loading

w- Jun 6, 2017 •
edited
Loading

w- Jun 6, 2017 •
edited
Loading

w- Jun 6, 2017 •
edited
Loading

dibyadas commented Jun 9, 2017 •
edited
Loading