WIP: better handling for server errors

[superdiana]

• Added exception classes to manage ClientErrors for Sms And Voice Api's. Adapted unit test to include exception testing.
• Added NotFoundError & BadRequestError to handle 404 & 400 Errors

[coveralls]

Coverage increased (+0.7%) to 96.844% when pulling e5c624ecd838ab128f0792118752554b1d6f0c69 on exceptions-error-handling into 0896a892e6ab6fddef7feef8a0108892254d6b77 on master.

[coveralls]

Coverage increased (+0.7%) to 96.844% when pulling e5c624ecd838ab128f0792118752554b1d6f0c69 on exceptions-error-handling into 0896a892e6ab6fddef7feef8a0108892254d6b77 on master.

[alexei]

It appears to me this PR not only falls short of addressing the original issue, but introduces new challenges.

First of all, the original issue raised a point about making a distinction between the various HTTP errors. This PR (a) adds support for 400 and 404 codes only, (b) completely ignores others (e.g. 429 which, ironically, is the reason why I started the conversation), and (c) makes it impossible hard for consumers to address these shortcomings (i.e. ExceptionHandler.__exceptions is private).

Second, the newly introduced NexmoError inherits ClientError so now we have another problem: distinguishing HTTP errors from Nexmo errors (which I presume are validation errors and should mostly fit in the HTTP bad request category). If the SDK wanted to have something like an "umbrella" type, wouldn't it be better to have NexmoError act as a base for all other exceptions?

Third, I think the implementation is on a dangerous path by introducing a new type for each code. If it were to add support for another code, it would add another type, right? But the PR introduces 21 new types, and only handles 2 of the 29 4xx codes. Additionally, consumers have no way of catching all HTTP client errors, all validation errors etc.

My original issue suggested the current ClientError and ServerError are fine as is, and that the SDK includes the status code as well, nothing more. So wouldn't it be easier to just add a property status_code on both ClientError and ServerError? The value can simply be copied from response.status_code. It addresses the issue while keeping the existing behavior as is, and consumers can deal with them if/as they see fit.

[superdiana]

@alexei Thank you for your input. We would love to see contributions and PR's for specific issues like yours. Although I didn't solve your issue in particular, you might have noticed (or not!) that I sorted the throttling error specifically and left the collection extensible as a work in progress to include more. One important thing to remind you of is that we have a code of conduct governing this repository and we foster a healthy environment so vocabulary and tone have to be in accordance to the minimum civility and good education norms.

As the advocate responsible for the SDK, I would love to accommodate all wishes but I'm afraid it isn't always possible. However, I suppose after your essay you will be proposing an interesting solution. Can't wait to see your PR, like I said before, we love community contributions.

[alexei]

@superdiana It appears there's a misunderstanding here: I thought this PR was fixing https://github.com/Nexmo/nexmo-python/issues/157 I must have been misled by your comments there saying that you were working on it and subsequently closing the issue. Hence my comment which, btw, I think was civil. Actually, considering my criticism of the implementation was clear, specific, and offered suggestions, I would even dare call it constructive.

That's the most I can offer at this time in terms of contributions. I expect the Nexmo team to be more intimate with both the API and the SDK, thus more suitable to addressing the issue. Besides, it appears the simple idea of having the exception object include the status code didn't win any hearts.

Since you're mentioning throttling, I now find it worthwhile pointing out the Nexmo API sometimes responds with HTTP 200 combined with a body like {...status: "1"...} (e.g. when sending a SMS), while other times it responds with HTTP 429 (e.g. when cancelling a number). There's also the non-standard HTTP 420 (e.g. when buying a new number) which is suspicious. So a client handling throttling would need to handle the new ThrottledError as well as employ some acrobatics to detect the 429 (and possibly the 420), thus going back to square one.

That said, I'd appreciate it if you re-read my comments, understood the point I'm making and the reasoning behind my suggestion.

Cheers!