Skip to content

Add a constructor to SendwithusError that stores content - #78

Merged
dlsteuer merged 1 commit into
sendwithus:masterfrom
DaleSeo:content-from-error
May 22, 2020
Merged

Add a constructor to SendwithusError that stores content#78
dlsteuer merged 1 commit into
sendwithus:masterfrom
DaleSeo:content-from-error

Conversation

@DaleSeo

@DaleSeo DaleSeo commented May 7, 2020

Copy link
Copy Markdown
Contributor

Description

Hi! I'm a user of this library and I'd like to make a suggestion to improve user experience around error handling.

Motivation and Context

As seen below, the content is being passed in when creating errors and raising them.

if response.status_code == 403:
raise AuthenticationError(content)
elif is_4xx_error:
raise APIError(content)
elif is_5xx_error:
raise ServerError(content)

On the other hand, none of the error classes doesn't implement their constructor.

class SendwithusError(Exception):
"""Base class for Sendwithus API errors"""
class AuthenticationError(SendwithusError):
"""API Authentication Failed"""
class APIError(SendwithusError):
"""4xx - Invalid Request (Client error)"""
class ServerError(SendwithusError):
"""5xx - Failed Request (Server error)"""

From the library users' perspective, this means they have to access the content of the errors like below, which is a bit error-prone (IndexError) and not very readable. It could be obvious to experienced Python developers who have a good understanding of how built-in Exception works internally but I had to spend quite some time to dig into your code to figure this out.

try: 
    api.get_template("INVALID") 
except sendwithus.exceptions.APIError as e: 
    raise MyCustomError(message=e.args[0])                                                                                                                                                                                                                                      

So, my suggestion is to add a constructor to SendwithusError that stores the content value in its content attribute. I think It would be great if the content of the errors could be accessed this way instead.

try: 
    api.get_template("INVALID") 
except sendwithus.exceptions.APIError as e: 
    raise MyCustomError(message=e.content)                                                                                                                                                                                                                                      

@DaleSeo
DaleSeo requested a review from a team as a code owner May 7, 2020 15:46
@demoore

demoore commented May 7, 2020

Copy link
Copy Markdown
Member

Thanks so much @DaleSeo! We'll have someone take a look at this.

@dlsteuer

Copy link
Copy Markdown
Contributor

Thanks for the contribution, looks good to me.

@dlsteuer
dlsteuer merged commit 23805d8 into sendwithus:master May 22, 2020
@DaleSeo
DaleSeo deleted the content-from-error branch May 22, 2020 15:51
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

Successfully merging this pull request may close these issues.

3 participants