SiftScience / sift-python

Sift API (Python client)
MIT License
20 stars 23 forks source link

Raise exceptions instead of returning them #42

Closed hmmbug closed 8 years ago

hmmbug commented 8 years ago

This pull request is to change the error behaviour for track(), score(), label() and unlabel() methods to be more pythonic in raising exceptions instead of returning them.

Currently it is necessary to call a method and check the response type as we don't know if we'll get a Response object come back or a subclass of Exception, eg. from the docs (https://siftscience.com/developers/docs/python/automation-apis/score-api):

response = client.score("USER_ID_HERE")
response.is_ok()            # false on failed score requests.

But if score() returns an exception then the is_ok() method on the second line doesn't exist and an AttributeError exception is raised.

A more complete way to error check would be this awkward mixture of try/except and if:

try:
    response = sift_client.score('me')
    if isinstance(response, Response):
        if response.is_ok():
            # yeah! success
        else:
            # method failed, but without exception
    elif isinstance(response, RequestException):
        # oh, no... this is a RequestException
except RuntimeError:
    # oh, no... this is a RuntimeError

but with the pull request we can just do it idiomatically:

try:
    response = sift_client.score('me')
    if response.is_ok():
        print 'it is ok to call is_ok() now'
except RuntimeError as e:
    # invalid args
except RequestException as e:
    # request failed
fredsadaghiani commented 8 years ago

@hmmbug This mostly LGTM. Could you please add a commit to remove the warning lines:

warnings.warn( ...

These are now superfluous and the call site can choose to log the exception. A few tests will also need to be updated.

This change will require a major version bump as it changes the contract for the APIs, but totally agree that this is more idiomatic Python.

Thanks!

hmmbug commented 8 years ago

Updated as per feedback above and Issue #43

@fredsadaghiani I've not updated the version number. I'll leave that for you & your colleagues to decide about 1.x or 2.0. Thanks.

fredsadaghiani commented 8 years ago

Thanks @hmmbug, this LGTM. I'm going to merge to master and add a few commits to cut a new release.

fredsadaghiani commented 8 years ago

FYI, v2.0.0.0 has been released: https://pypi.python.org/pypi/Sift