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

slumber.Resource._process_response #112

Open
diefans opened this issue May 25, 2015 · 2 comments
Open

slumber.Resource._process_response #112

diefans opened this issue May 25, 2015 · 2 comments

Comments

@diefans
Copy link

diefans commented May 25, 2015

I am just checking out your code and stumbled upon slumber.Resource._process_response which seems to throw away and ignore responses =! 200..299.

    def _process_response(self, resp):
        # TODO: something to expose headers and status

        if 200 <= resp.status_code <= 299:
            return self._try_to_serialize_response(resp)
        else:
            return  # @@@ We should probably do some sort of error here? (Is this even possible?)

I think there are a lot of REST APIs which supply meaningful information beyond 2xx and the API wrapper should never try to make assumptions about the API it is wrapping.

@samgiles
Copy link
Owner

Agreed, although it's hard to know what the right thing to do is really. I only see one option, but this is also making some assumptions. Maybe it's the best this API can do:

Throw an exception pertaining to the error code range: 4xx, 5xx. With this however, how do we handle the body content, if any, that might provide meaningful information (an assumption unfortunately), e.g. The API might return some helpful information as part of a 410 gone response for example. Maybe we could follow something like algorithm:

if not (200 <= resp.status_code <= 299):
    body = None
    try:
        body = self._try_to_serialize_response(resp)
    except:
        pass
    raise HTTPClientError(resp.status_code, body)

Thoughts?

@diefans
Copy link
Author

diefans commented May 29, 2015

Every HTTP REST API delivers its response by a tuple of (status code, encoded body). Slumber is basically focusing on deserializing the body into a python structure but ignores the status code. From my point of view all status codes should be handled in the same way - no assumption if this and that are considered an error. The status code must be passed to the application on every case, since the application is implementing the wrapped API and not slumber, so in the end the application must turn (status code, encoded body) into an exception or an error value. In that way Slumber would be a transparent tool without assumptions.

So I think of something like this:

    def _process_response(self, resp):
        result = self._try_to_serialize_response(resp)
        result.status_code = resp.status_code
        return result

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

No branches or pull requests

2 participants