python-http-client should define it's own error class which can be imported by consuming libraries

[w-]

Issue Summary

If the response is not success (i.e not HTTP2xx) the library throws a urllib HTTPError.
For clients consuming this library, they now need to know that it was implemented in urllib, import that module into their application and handle the error.

Ideally, a consuming application only needs to know about python-http-client and not how urllib was implemented in order to handle common place http errors.

Steps to Reproduce

1. make an api call that returns anything other than HTTP2xx
2. attempt to handle error raised without importing urllib and without doing a catch all.

Technical details:

• python-http-client Version: master (latest commit: [commit number])
• Python Version: all

[thinkingserious]

Please see sendgrid/sendgrid-python#316

Thanks!

[w-]

@thinkingserious don't think this shojuld be closed. please see sendgrid/sendgrid-python#315 (comment)

[thinkingserious]

Hi @w-,

It looks like this functionality was just added in the latest commit. Do you mind taking a look?

I have not yet fully reviewed the PR, but this functionality will be included before we merge it.

Thanks!

[w-]

@thinkingserious can you help provide a direct link?
I went through the commit list but nothing obvious jumped out

thanks

[thinkingserious]

https://github.com/dibyadas/sendgrid-python/blob/3dd7ef3695d68e8d7a18ab3d0973be6fe8b6ca0c/sendgrid/sendgrid.py#L69

[w-]

i see. i thought you were talking about latest commit to this library.
I think both of you guys may be missing the point i'm attempting to communicate.

I believe that there needs to be a better implementation around how errors are raised from the underlying python-http-client library to the sendgrid-python library and ultimately to the consuming code.

I've left some additional comments
sendgrid/sendgrid-python#316

[thinkingserious]

Hi @w-,

Thank you for the continued feedback.

Could you provide an example of where you would make changes in this repo? That would probably help clarify. Thanks in advance :)

With Best Regards,

Elmer

[w-]

As i mentioned in other comments, because of the way python-http-client is implemented i'm unsure.

maybe here?
https://github.com/sendgrid/python-http-client/blob/master/python_http_client/client.py#L138

so we would define a HttpClientError similar to what i define here
sendgrid/sendgrid-python#316 (comment)

and we would wrap that return statement like so:

try:
    return opener.open(request)
except urllib.HttpError as err:
    raise HttpClientError(err.url, err.code, err.hdrs, err.read())

now a consuming library only needs to be aware of HttpClientError which it can import from python-http-client.
also, in tracebacks we don't see it point to underlying code in urllib (why would a consumer ever care?)

The reason we want a structured exception instead of a generic one is so we can add logic to our exception handling at the sendgrid-python level. a prime case is differentiating between regular API Errors and Rate Limiting errors. Or permission vs bad inputs. etc.

[w-]

as an aside, i'm also unsure that throwing an exception on http error is the way to go.

I think it can also be considered normal to receive a non success HTTP response and let the consumer decide how they want to handle it.
so maybe instead of throwing an error, python-http-client, just returns an HTTP response?

not sure about that either.

[thinkingserious]

I think it can also be considered normal to receive a non success HTTP response and let the consumer decide how they want to handle it.

How about we raise something like ConnectionError, Timeout, HTTPError and TooManyRedirects similar to requests? (But, I think we can include Timeout in ConnectionError for simplicity)

[dibyadas]

I second @w- on this. If we are having our own library, we might as well define our own exceptions. This way, anyone who uses this library is not bothered about any underlying implementations.
So then, the sendgrid-python should be aware of only python-http-client and not urllib.

[thinkingserious]

@dibyadas,

Do you want to give it a try with a PR? Or would you like me to give it a shot?

[dibyadas]

@thinkingserious I would love to work on this! I can give it a try and you can guide me and suggest changes and improvements. 😄
What do you say?

[thinkingserious]

Awesome, thanks!