Comments in client.py are incorrect about return types and exceptions

[mrcoles]

The comments in the client.py for Client.track (and likely the other methods) are incorrect. They say:

        Returns:
            A requests.Response object if the track call succeeded, otherwise
            a subclass of requests.exceptions.RequestException indicating the
            exception that occurred.

Here’s the current revision: https://github.com/SiftScience/sift-python/blob/ef04a0fa655da36f0a7bcaa341df930bcb282d5a/sift/client.py

1. The method returns an instance of sift.client.Response, not requests.Response
2. If the requests call raises an exception, yes it looks like it returns requests.exceptions.RequestException
3. Undocumented, it looks like instantiating a sift.client.Response instance can lead to a sift.client. ApiException being raised.

It appears the comments in this file need to be updated to reflect the proper return types and to also identify the ApiException that can be raised.

[hmmbug]

Hi, this is related to my pull request which updates the client to only return Responses or raise Exceptions, although I perhaps didn't correct the docs to be as accurate as your observations.

[fredsadaghiani]

For reference, the PR #42.

[mrcoles]

Does that PR address the ApiException or does it just blanket it into the RuntimeException? The client should strive to know & share exactly what exceptions it can raise.

My code currently looks something along the lines of the following below. It was suggested to me to add a retry for ApiException. I also have a concept of a short-circuit via encountered_error, to not make any further Sift calls within a request once one call has failed (e.g., if the sift endpoint is down and I need to make several calls during a request, I would rather fail on the first and not try again).

import sift

client = sift.Client(api_key=settings.SIFT_REST_API_KEY)

retries_remaining = 1
encountered_error = False

while retries_remaining >= 0:
    try:
        response = client.track(event, props, return_action=return_action)
    except sift.client.ApiException as e:
        # ...log something here
        if retries_remaining <= 0:
            encountered_error = True
            response = None
        retries_remaining -= 1
    else:
        if isinstance(response, requests.exceptions.RequestException):
            # ...log something here
            encountered_error = True
            response = None
        elif not response.is_ok():
            # ...log something here
            encountered_error = True
            response = None

        break

if response is not None:
    pass

Also, whatever update happens to the client, it should probably be a major or minor bump, not bugfix, since the way you use the client can break, e.g., 1.1.2.6 -> 1.2.0 or 2.0.0 (maybe someone more pedantic than I has a specific opinion on that)

[fredsadaghiani]

@mrcoles Agreed on the major version bump. See this comment on #42 where I make the same remark.

@hmmbug This is good feedback. We should only raise sift.client.ApiException to keep the call sites clear and straightforward.

[hmmbug]

Pull #42 updated. Please review.

[fredsadaghiani]

44 upgrades to v2.0.0.0 and changes the API contract such that sift.client.ApiException is raised in the case of request error.

[fredsadaghiani]

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