Candidate "improve manual redirect-walking" core API change.

Sorry for dropping off the face of the earth for a while, folks.  After reading the reaction to my previous pull request, I realized that the bugfixes I want to get in will be easier, cleaner, and make more sense in context if I make the big API change _first_.  So this is the proposed big API change.  It is not fully baked -- if nothing else, it needs tests -- but I'd like to get your opinion on the idea first.
- Session.send now offers a new mode, `iter_redirects=True`, in which
  it returns an iterator over redirects _instead of_ the first response.
  In this mode, the first request does not actually fire until you call
  next() on the iterator the first time.  Unlike the legacy
  Session.resolve_redirects, the first response is included in the
  generated sequence.
- Session.resolve_redirects is preserved and works as it always has, but
  is now clearly documented as not the API you want.  (The docstring for
  Session.send itself probably needs some improvement as well.)  Its
  calling convention has been slightly polished: the request argument is
  now optional (defaults to `resp.redirect`), it will accept arbitrary
  kwargs to be passed to the adapter, and it defaults the same set of kwargs
  from session settings as `send` itself does.
- The `allow_redirects=False` mode of `Session.send` also still works just
  as it always has.  If both allow_redirects=False and iter_redirects=True
  are specified, allow_redirects=False wins.
- SessionRedirectMixin has been replaced by a RedirectIterator class
  which is not a parent of Session.
- Response.history is now always a list.


> Session.send now offers a new mode, `iter_redirects=True`, in which
> it returns an iterator over redirects instead of the first response. [...] If both allow_redirects=False and iter_redirects=True are specified, allow_redirects=False wins.

Aah! Nope, that's not the way to do this. That last sentence should be a bit of a clue: your arguments to a function should never be fighting each other.

More importantly, function argument should not change the type of the returned value. This makes it substantially more difficult to reason about the function. If you really want to do this then you're going to have to have a new function call here, rather than a new parameter.

@kennethreitz We're going to want your insight here. =)


I agree with @Lukasa's comment. Further, `Session#send` should not return either an iterator or a response. It should only return one ever. The 98% use case desires a Response and that's all we should ever return. I will not budge on this.

I haven't reviewed the code at all, but I will probably leave PR review when I do. As this is described right now, I'm :-1: on the entire change with the caveat that I haven't reviewed much beyond your description @zackw 


I think that insisting `Session.send` only ever return one type of object is excessively dogmatic, but I acknowledge that the toggle-switch arguments that conflict are annoying.

Perhaps this is more palatable?
- `Session.resolve_redirects` is deprecated.
- `Session.send` remains as is, but the `allow_redirects=False` mode is deprecated.
- New method `Session.send_no_redirect` does what `Session.send` with `allow_redirects=False` does now.
- New method `Session.send_iter_redirect` returns the proposed redirect iterator.
- New method `Session.prepare_request_for_redirect` takes a response and returns a new PreparedRequest to follow the redirect.

I've started implementing this on a new branch at zackw/requests@6703df73e6 to see how it goes.  I probably won't be done till tomorrow or Friday.

Better names for the new `send_*` variants solicited.


I'm not really happy with three new methods, I'm afraid: it feels like excessive complexity.

I don't understand why we're making this so complicated. Why can't we piggyback on what we already have? With `allow_redirects=False`, you'll potentially get back a redirect (which will satisfy your `Response.is_redirect` predicate). It should then be possible to simply feed it to a method on the session (e.g. `Session.follow_redirect()`) which takes all the same parameters as `Session.send`. What's your rationale for not going that way?


> I don't understand why we're making this so complicated. Why can't we piggyback on what we
> already have? With allow_redirects=False, you'll potentially get back a redirect (which will satisfy
> your Response.is_redirect predicate). It should then be possible to simply feed it to a method on
> the session (e.g. Session.follow_redirect()) which takes all the same parameters as Session.send.
> What's your rationale for not going that way?

Fundamentally, the issue I'm trying to fix is that `Session.resolve_redirects` doesn't include the very first response in its generated sequence.  That makes it difficult to use correctly even after all its superficial problems (not taking arbitrary kwargs, allowing you to shoot yourself in the foot by passing the wrong request object as the second argument) are corrected.  You wind up having to write code like this:

```
first_resp = sess.send(..., allow_redirects=False)
process_response(first_resp)
for resp in sess.resolve_redirects(first_resp, ...):
    process_response(resp)
```

and if what `process_response` does cannot be extracted to its own function, for whatever reason (e.g. nontrivial exception goo), you may find yourself duplicating tens of lines of code.

What I _want_ as an API is

```
for resp in sess.send(..., iter_redirects=True):
    process_response(resp)
```

but since `iter_redirects=True` has been nixed, and `resolve_redirects` cannot itself be changed, we gotta make up a new name.  And if we're making up a new name for a new redirect-related operating mode of `send`, it is more globally consistent to give `allow_redirects=False` mode its own method name as well.

If I understand correctly what you are suggesting, it amounts to

```
resp = sess.send(req, ..., allow_redirects=False)
while resp.is_redirect:
    process_response(resp)
    resp = sess.send(sess.prepare_request_for_redirect(resp), ..., allow_redirects=False)
process_response(resp)
```

which is more typing and doesn't get rid of the need to process the response in two places (textually).


Well, firstly, it doesn't require us to process the response in two places:

``` python
resp = sess.send(...)
while True:
    process_response(resp)
    if not resp.is_redirect:
        break
```

Nevertheless, we still don't need three new methods, we only need one: `Session.send_iter` (or equivalent). This will involve rewriting `Session.send` to basically just exhaust `Session.send_iter`. We should not be changing what `Session.send` does by default (which is to follow all redirects), as this is overwhelmingly the most common use case. Your use case is a valuable one, and we want to support it, but we should not be making it the primary interface or causing pain to the users who are happily using the current interface.


I did not propose to change `Session.send` in the default mode, only to excise the `allow_redirects=False` mode to its own method.  Please see f4d7bc780d57de4e06651fa2cd4cb790c0ac4789.


Right, but I see no reason to do that either. People who are currently handling their own redirects should not be forced to switch to the new scheme. Especially as if `Session.send()` and `Session.send_iter()` have the same underlying code, `Session.send()` with `allow_redirects=False` can be a simple special case where we only iterate over the iterator once. I don't see that the extra methods buy us anything we don't already have.


I see where you're coming from?  Only it turns out that the code is internally tidiest if `Session.send_no_redirect` (in my latest code I'm calling it `send_single`) exists as a separate method, at which point we are only arguing over whether it should be part of the exposed API.  I happen to think that we should take the opportunity to deprecate `allow_redirects=False`, which is a confusing name (it sounds to me like it would cause an _error_ upon encountering a redirect).


Ok, so this is a disagreement about API design. I strongly suggest we keep `allow_redirects`. It's been present in the API for a very long time, large quantities of both formal and informal documentation refer to it, and it would be jarring and painful to switch away from it. Additionally, it turns this from a minor release into a major one by being backward incompatible. =)


Are you okay with keeping it but applying a DeprecationWarning?  That being what I actually did. ;-)


I am not. =) Requests does not have an official deprecation policy, but if we did I'd be hugely reluctant to throw deprecation warnings into minor releases. Additionally, `DeprecationWarning`s are silent by default in 2.7 onward.

Regardless, even if they were noisy, I'd be against this change: it adds needless complexity and trashes the interface for no good reason. The parameter works fine as is, provides no engineering difficulty to keep, and allows a seamless API transition.


I agree with all of @Lukasa's feedback. Frankly @zackw you seem to want to throw everything out and rewrite it from scratch. While that isn't always a bad thing, you seem to be taking advantage of our desire to help you out. I for one will not accept that. We have tried to help usher you through the process of making your work as close to perfect as possible and all I have seen in this thread is you fighting us. I understand why you're fighting but I see no genuine attempts at compromise. At best you're not making me anymore sympathetic to your goal.


Uh, you realize you just nitpicked a pull request that I already junked
based on Cory's feedback? I will make sure of it tomorrow, but I believe
all of the things you didn't like are already better in the new (still WIP)
redirection-generator-2 branch, or else they were preeexisting conditions
in code that I just moved. (The _cookies thing, for instance: I didn't do
that. That was there when I got here. It looks wrong to me too, but I don't
understand what it's doing so I left it alone.)

On the larger issue: My idea of good style is, it has become clear,
radically different than yours. This is your project and therefore I am
doing my damnedest to emulate your style, but the difference is great
enough that I will inevitably get it wrong at least some of the time. I
also assure you that I am not just changing things for the sake of changing
things. I am trying to fix real bugs which I personally tripped over, and
some of them need API changes to fix. I appreciate your collective patience
to date, and if there's anything I can do to make this easier for everyone
I will consider it. For instance, it seemed pointless to officially
file all the bugs on the list in my head when I could just send the fixes,
but if it would make it easier for you to keep track of what the point is
here, I can do that. I'd also be happy to turn up on IRC for a planning
session if that would help.


> Uh, you realize you just nitpicked a pull request that I already junked based on Cory's feedback?

You made no indication that you were junking it. You didn't close it and you didn't indicate you were working on yet another branch. I won't even address your "nitpicking" claim. That isn't productive.

> Uh, you realize you just nitpicked a pull request that I already junked based on Cory's feedback?

Sorry but you can probably understand why I would think this was something you had done.

> My idea of good style is, it has become clear, radically different than yours.

Most of it isn't even just style. Some of it is just recognizing an anti-pattern when you see it. There is absolutely no need for a class method on the iterator class. Beyond that, class methods should basically always (in Python) be public especially when used in a public manner. The naming convention in the Python community is not just our "idea" of good style, it's a rather global convention. A function (or attribute) prefixed with an `_` is meant to be private and an implementation detail. It doesn't matter that everything is public in Python, the intent on the behalf of the author is communicated by those `_`s and indicate that the function is an implementation detail subject to change at will. If you're consuming that function publicly then you're just inviting disaster on yourself and it isn't the author's fault. 

> I am trying to fix real bugs which I personally tripped over, and some of them need API changes to fix. [snip] For instance, it seemed pointless to officially file all the bugs on the list in my head when I could just send the fixes, but if it would make it easier for you to keep track of what the point is here, I can do that.

It's not pointless to us. The only "bug" that I've seen fixed is the inconsistency in the type of `Response`'s `history` attribute. I have no other understanding of bugs in this section and I'm not a psychic. Also none of your changes to date have been evident that they fix any specific bugs. In fact they have been all API changes. Perhaps your concept of "bugs" are difficulties with what (I admit freely) is an imperfect API for handling redirects yourself. It is acceptable to improve the API but when I look at this all I see is a giant reifying of things.

If the bugs make sense to you to be grouped together instead of sitting through and filing 100 issues on GitHub then group them. At least we can discuss the merit and core problem of each of them and discuss possible solutions before you spend any more time working on these pull requests that you essentially just keep closing and totally rewriting. This seems like a waste of your time as well as ours  whenever we try to review them. I have more projects to tend to than just requests. I review your code not to nitpick but to help it have a chance of being meged. If you see it as nitpicking then I'm not sure I can help you.

You might also benefit from reading about [30% Feedback](http://blog.42floors.com/thirty-percent-feedback/).


Also, for what it is worth, having a huge change like this in one commit is not helpful to either @Lukasa or me. This [Guide to Git Commits](https://wiki.openstack.org/wiki/GitCommitMessages) for OpenStack should explain why it's harder for us to review one huge commit as opposed to a series of smaller ones which tell a story.


Alright enough fighting guys :)

---

@zachw, would you like to have a Skype or Google Hangout sometime this week or next? Perhaps I could really connect get a good idea for what you're trying to accomplish, and see if I can come up with a supplemental API that would work well :)


@kennethreitz I'm @zackw, not @zachw :-)

Could probably sit down and talk sometime tomorrow.  I'm in US/Eastern and can do either Skype or Google (but Skype is preferred).  I will also find time to file bugs for all the things I was trying to fix, as requested earlier.