Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweaks to exception handling #135

Merged
merged 3 commits into from
Jan 13, 2017
Merged

Tweaks to exception handling #135

merged 3 commits into from
Jan 13, 2017

Conversation

richleland
Copy link
Contributor

@richleland richleland commented Jan 12, 2017

This PR does a few things:

Traceback (most recent call last):
  File "send_transmission.py", line 19, in <module>
    campaign='python-sparkpost example',
  File "/Users/rleland/src/oss/python-sparkpost/sparkpost/transmissions.py", line 248, in send
    results = self.request('POST', self.uri, data=json.dumps(payload))
  File "/Users/rleland/src/oss/python-sparkpost/sparkpost/base.py", line 41, in request
    **kwargs)
  File "/Users/rleland/src/oss/python-sparkpost/sparkpost/base.py", line 16, in request
    raise SparkPostAPIException(response)
  File "/Users/rleland/src/oss/python-sparkpost/sparkpost/exceptions.py", line 30, in __init__
    errors='\n'.join(errors)
TypeError: sequence item 0: expected string, dict found

This was caused because of how we were constructing the message. Error code, when present, would be parsed as an int, and didn't work with concatenation. I modified it to use string formatting.

  • Intercepts when there is an issue parsing the JSON, passing back the repsonse body as the error message
  • Fixes an error in a transmissions test
  • Adds a test for a response that contains no JSON

' Description: ' + e.get('description', '') +
'\n'
error_template = "{message} Code: {code} Description: {desc} \n"
errors = [error_template.format(message=e['message'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we use e.get('message') here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we could - though if message isn't there this is useless to users - I could go either way w/it

@rajumsys
Copy link
Contributor

Intercepts when there is an issue parsing the JSON, passing back the repsonse body as the error message

When will we be unable to parse json? Does it mean our api is returning non-json strings?

@richleland
Copy link
Contributor Author

@rajumsys yes we account for a responses that may not return JSON as the body - early on we had cases where nginx didn't intercept and provide JSON, but I don't think there's a compelling reason to remove it - not sure if nginx covers all error scenarios.

@richleland richleland merged commit f3b27cc into master Jan 13, 2017
@richleland richleland deleted the issue-134 branch January 13, 2017 20:46
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.

2 participants