closeio / closeio-api

Python API Client for Close
http://developer.close.com/
MIT License
65 stars 47 forks source link

APIError message should contain the response's status code #86

Closed wojcikstefan closed 8 months ago

wojcikstefan commented 7 years ago

So that it's obvious at a glance what kind of an error we experienced. Right now it's possible to get an enigmatic traceback:

In [3]: api.get('me')
Traceback (most recent call last)
<ipython-input-3-842652bd220e> in <module>()
----> 1 api.get('me')

/Users/wojcikstefan/Repos/closeio-api/closeio_api/__init__.py in get(self, endpoint, params, **kwargs)
    132         """
    133         kwargs.update({'params': params})
--> 134         return self._dispatch('get', endpoint+'/', **kwargs)
    135
    136     def post(self, endpoint, data, **kwargs):

/Users/wojcikstefan/Repos/closeio-api/closeio_api/__init__.py in _dispatch(self, method_name, endpoint, api_key, data, debug, **kwargs)
    108             raise ValidationError(response)
    109         else:
--> 110             raise APIError(response)
    111
    112     def _get_rate_limit_sleep_time(self, response):

APIError:
wojcikstefan commented 7 years ago

Note that this is a breaking change as unicode(exc) no longer contains just a stringified json for validation errors. That said, we should encourage people to use exc.errors and exc.field_errors anyway.

wojcikstefan commented 7 years ago

@thomasst would you please do a quick review of this one?

wojcikstefan commented 6 years ago

I'm not aware of any. However, to introduce this breaking change in a mature way, I think we should bump the major part of this package's version after this is merged, and note in the release that one should use json.loads(exc.response.text) from now on. Sounds good @philfreo @thomasst ?

thomasst commented 6 years ago

and note in the release that one should use json.loads(exc.response.text)

No, they shouldn't have to JSON-parse the text property. There's already exc.errors and exc.field_errors for ValidationError. Are there any other cases we should cover?

wojcikstefan commented 2 years ago

@eengoron is this something we still want to do? If no, feel free to close this PR. If yes, but there's a better way to do it, please close this PR and open a new issue/PR.