Added support for error handling #17

dibyadas · 2017-05-24T13:27:43Z

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.

SendGridDX · 2017-05-24T13:28:00Z

All committers have signed the CLA.

dibyadas · 2017-05-24T13:33:23Z

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)

dibyadas · 2017-05-24T13:34:42Z

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

w- · 2017-05-24T17:18:41Z

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.

thinkingserious · 2017-05-26T17:15:09Z

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

thinkingserious · 2017-06-01T20:27:40Z

@w-,

Do you have time to take a look? Thanks!

dibyadas · 2017-06-02T07:59:59Z

@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

thinkingserious · 2017-06-02T13:38:18Z

@dibyadas,

Please add the test in the existing file. Thanks!

w- · 2017-06-06T11:05:41Z

python_http_client/__init__.py

@@ -1 +1,2 @@
 from .client import Client
+from .exceptions import *


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

w- · 2017-06-06T11:06:37Z

python_http_client/exceptions.py

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


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

w- · 2017-06-06T11:07:18Z

python_http_client/exceptions.py

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


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.

w-

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

w- · 2017-06-06T11:07:45Z

python_http_client/exceptions.py

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


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

w- · 2017-06-06T11:08:42Z

python_http_client/exceptions.py

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


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! :)

w- · 2017-06-06T11:09:21Z

python_http_client/exceptions.py

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


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.

dibyadas · 2017-06-06T16:03:16Z

python_http_client/client.py

+        try:
+            return opener.open(request)
+        except HTTPError as err:
+            handle_error(err)


@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?

thinkingserious · 2017-06-07T05:21:32Z

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

dibyadas · 2017-06-09T15:53:52Z

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

dibyadas · 2017-06-16T08:02:53Z

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

thinkingserious · 2017-06-16T21:55:27Z

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

w- · 2017-06-19T01:57:50Z

I'm ok with this.

thinkingserious · 2017-06-21T02:34:17Z

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

dibyadas · 2017-06-21T04:33:40Z

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

iandouglas · 2017-09-15T16:42:04Z

Per https://github.com/sendgrid/sendgrid-python/issues/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.

andriisoldatenko

Please check code using flake8/pep8

andriisoldatenko · 2017-09-15T17:30:15Z

python_http_client/exceptions.py

+	pass
+
+class MethodNotAllowedError(HTTPError):
+	pass


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

andriisoldatenko reviewed Sep 15, 2017

View reviewed changes

		@@ -1 +1,2 @@
		from .client import Client
		from .exceptions import *

Added support for error handling #17

Added support for error handling #17

Conversation

dibyadas commented May 24, 2017 • edited

SendGridDX commented May 24, 2017 • edited

dibyadas commented May 24, 2017

dibyadas commented May 24, 2017

w- commented May 24, 2017

thinkingserious commented May 26, 2017

thinkingserious commented Jun 1, 2017

dibyadas commented Jun 2, 2017

thinkingserious commented Jun 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w- Jun 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w- left a comment

Choose a reason for hiding this comment

w- Jun 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w- Jun 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thinkingserious commented Jun 7, 2017

dibyadas commented Jun 9, 2017 • edited

dibyadas commented Jun 16, 2017

thinkingserious commented Jun 16, 2017

w- commented Jun 19, 2017

thinkingserious commented Jun 21, 2017

dibyadas commented Jun 21, 2017

iandouglas commented Sep 15, 2017

andriisoldatenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyadas commented May 24, 2017 •

edited

SendGridDX commented May 24, 2017 •

edited

w- Jun 6, 2017 •

edited

w- Jun 6, 2017 •

edited

w- Jun 6, 2017 •

edited

dibyadas commented Jun 9, 2017 •

edited