[RFC] Change behaviour of Response.ok in 3.0

# Preamble

Please note that this a request for comments, not something that we will _definitely_ do in Requests.
# Problem

Right now we have an attribute defined on a Response object, `ok`. This attribute [currently](https://github.com/kennethreitz/requests/blob/9bbab338fdbb562b923ba2d8a80f0bfba697fa41/requests/models.py#L617..L623) calls `self.raise_for_status()` and catches the [exception](https://github.com/kennethreitz/requests/blob/9bbab338fdbb562b923ba2d8a80f0bfba697fa41/requests/models.py#L825..L837) raised. `raise_for_status` appropriately only raises an exception for status codes in the 4xx or 5xx range. This means that a Response with a status code in the 2xx and 3xx range will be "ok". The problem is that "ok" has a certain association in HTTP with 2xx responses, specifically 200 responses. This _may_ (I haven't looked to see if anyone has had problems with this) be misleading, especially if the user is combining their usage of the `ok` attribute with `allow_redirects=False`.
# Proposed Solution

Instead of using `raise_for_status` to determine the "ok-ness" of a response, we should compare the status code of the response directly. This will do two things:
1. It will narrow the definition of `Response.ok`
2. It will make using `Response.ok` faster. Currently we add a new stack, potentially throw and catch an exception, and then return. With a simple comparison, we could just do: `return 200 <= self.status_code < 300` which is much faster. This argument, however, doesn't hold much weight for me personally.

I'd really love @Lukasa and @kennethreitz's opinions here as well as anyone else willing to share their opinion publicly.


I think that `ok` is just poorly named. I'd be more interested in having predicates that cover the major response ranges: `response.success` , `response.redirect`, `response.client_error`, `response.server_error`. We could then have overlap fields, like `response.error` (a superset of `client_error` and `server_error`).

That said, if we think that's too heavyweight, I'm still in favour of reducing the scope of `response.ok`.


Yeah I agree that `ok` is poorly named. I'm not sure a bunch of new attributes is the right way to do this, but I'm not against it. We already have `response.is_redirect()` which, to be fair, could/should be changed to an attribute.


Against a bunch of new attributes. I'm all in favor of people doing a direct comparison themselves.

I think the current implementation is fine, and also allows for some flexibility when it comes to creative connection abstractions. Not sure if that's a good thing to do or not. 

Perhaps a better way to go about this would be to add a parameter to raise (e.g. `raise_for_status(inline=True)`, which would return True/False) .


Alternate proposal:

Step 1: Heavily document the behaviour of the `ok` attribute, cache the result on the first run (like `content`)

Step 2: Add to the status_codes module a sort of Python 3-esque range class. In Python 3, range returns a range object which allows you to easily do `200 in range(200, 300)` without iterative over the whole thing. We could do something similar in `status_codes` that would be cheap and work on Python 2 and 3 the exact same way (instead of creating a bunch of ~99 item lists at import.

Alternatively, provide lists of the known status codes, e.g., `success = [200, 201, ..., 208, 226]` The latter is a bit more maintenance and the former is a bit magical, albeit very simple.


@sigmavirus24 that sounds great to me. 

`if response.status_code in requests.codes.success`


Love it.

I propose renaming this issue to be clearer, and marking it 'contributor friendly'. It's a nice starting issue for someone. =)


Hi, I was playing with this issue and there's one thing I noticed. The response for the 3xx status codes isn't uniform. A GET with codes 301, 302, 303 and 307 ends up with a 200 response. The other ones receive the actual code (304/304 and so on). I tested this with httpbin/status/code_number.

If the issue is going to be solved this way, the success attribute must account for this behavior, right?


@Itwilloutliveyou I'm sorry, I don't think I fully understand what you're getting at here: can you elaborate or rephrase?


The solution proposed is to add a success attribute listing all the status codes that are considered ok (200, 201, 202, ..., 226). Then, we compare the status code of the response with the ones listed in success.

What happens is that the response for a 301 redirect, for example, is a status code 200. The same happens for other status codes of the 3xx family. @sigmavirus24 came with the idea that one of the reasons for that change is to narrow the definition of Response.ok. If that's the case, wouldn't it be better to rethink the outcome of those 3xx cases?


I think @sigmavirus24's point is just that 3XX status codes should probably not be considered "ok": they aren't. But requests _always_ follows redirects, so the only way to _end up_ at a 3XX status code is if the 3XX code is not an unambiguous redirect: i.e., it does not have a `Location` header.

The reason your testing showed no following for 304 and friends is because 304 is not a defined HTTP status code, so httpbin was not generating a Location header for it. That means requests ends up with the 304 code and cannot follow it any further. @sigmavirus24 rightly proposes that that response should not be "ok": it is almost certainly not.


I see, if a 301 redirect ends up on a successful retrieval of some
resource, then the final status is a 200 code.

Alright, my attempt is probably right. Tonight I'll make the PR, hope
everything is ok!

Thanks!
Am 16.05.2016 04:10 schrieb "Cory Benfield" notifications@github.com:

> I think @sigmavirus24 https://github.com/sigmavirus24's point is just
> that 3XX status codes should probably not be considered "ok": they aren't.
> But requests _always_ follows redirects, so the only way to _end up_ at a
> 3XX status code is if the 3XX code is not an unambiguous redirect: i.e., it
> does not have a Location header.
> 
> The reason your testing showed no following for 304 and friends is because
> 304 is not a defined HTTP status code, so httpbin was not generating a
> Location header for it. That means requests ends up with the 304 code and
> cannot follow it any further. @sigmavirus24
> https://github.com/sigmavirus24 right proposes that that response
> should not be "ok": it is almost certainly not.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly or view it on GitHub
> https://github.com/kennethreitz/requests/issues/2645#issuecomment-219363390


@Lukasa when designed, 'ok' meant 'not an error'. 300 is not an error. I still think should be the case. 

e.g. '.ok' isnt OMG ALL IS PERFECT, it is NOTHING APPEARS IMMEDIATELY BROKEN


so, with that logic, a redirect is a perfectly valid and ok response.

we already have `is_redirect`, so a `success` version of that would prob be fine to add. but we shouldn't change the conceptual behavior of `ok`.


if only there was a 'maybe' boolean value


```
>>> r.ok
0.9
```


What about:
- `response.status.is_success()`
- `response.status.is_redirect()`
- ...
- `int(response.status)` for converting to `response.status_code`.


> e.g. '.ok' isnt OMG ALL IS PERFECT, it is NOTHING APPEARS IMMEDIATELY BROKEN

That's what I thought it meant too, but that doesn't come across to me.

Just to play Devil's Advocate on this topic and tie it in with the boolean issue-

As far as I'm concerned, unless it only signifies a HTTP 200 OK, every response that is *properly formed* is `ok` and should not evaluate in a boolean context to `not True`.

If I receive a properly formed 404 or 500, awesome. That is `ok` by me, because the response might have a redirect history or contain useful information in the headers or content.  To me, a "not ok" response is one that is improperly formed, but hasn't raised an exception (yet).

For some context:

* If `requests` processes a URL shortener that causes multiple redirects, but ends on a 4xx or 5xx page, the `response` is `not True` and `not ok` -- yet it has a rich history that gets totally blown away **unless** someone reads a lot of documentation.

* If one encounters a response with 4xx with a body containing info about rate-limiting or authorization, it is `not True` and `not ok`, and that information also might be missed unless someone really reads the docs.

The current behavior and some of the above changes don't seem appropriate for `response.ok` as they would still be misleading to a lot of people; many  of the above suggestions seem better implemented as `response.http_okay` and `response.http_okayish` -- where it is semantically clear that the http code is "okayish", as the http code could not be okay but the response itself is fine.

Honest to goodness I just consider this entire issue an excellent example of why `.ok` is an unsuccessful property name, and why responses should not be boolean. There are too many entirely reasonable reads of what a response being "ok" might mean.

In an effort to clean up the issue tracker to only have issues that are still relevant to the project we've done a quick pass and decided this issue may no longer be relevant for a variety of potential reasons, including:

* Applies to a much older version, unclear whether the issue still applies.
* Change requires a backwards incompatible release and it's unclear if the benefits are worth the migration effort from the community.
* There isn't a clear demand from the community on the change landing in Requests.

If you think the issue should remain open, please comment so below or open a new issue and link back to the original issue. Again, thank you for opening the issue and for the discussion, it's much appreciated.