Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

zackw commented Feb 26, 2014

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.
@zackw zackw Candidate "improve manual redirect-walking" core API change.
 * 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.)

 * 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.
e2af428
Owner

Lukasa commented Feb 26, 2014

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. =)

Owner

sigmavirus24 commented Feb 26, 2014

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 👎 on the entire change with the caveat that I haven't reviewed much beyond your description @zackw

Contributor

zackw commented Feb 26, 2014

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@6703df7 to see how it goes. I probably won't be done till tomorrow or Friday.

Better names for the new send_* variants solicited.

Owner

Lukasa commented Feb 26, 2014

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?

Contributor

zackw commented Feb 26, 2014

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).

Owner

Lukasa commented Feb 26, 2014

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

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.

Contributor

zackw commented Feb 26, 2014

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 f4d7bc7.

Owner

Lukasa commented Feb 26, 2014

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.

Contributor

zackw commented Feb 26, 2014

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).

Owner

Lukasa commented Feb 26, 2014

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. =)

Contributor

zackw commented Feb 26, 2014

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

Owner

Lukasa commented Feb 26, 2014

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, DeprecationWarnings 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.

@sigmavirus24 sigmavirus24 commented on the diff Feb 27, 2014

requests/sessions.py
-class SessionRedirectMixin(object):
- def resolve_redirects(self, resp, req, stream=False, timeout=None,
- verify=True, cert=None, proxies=None):
- """Receives a Response. Returns a generator of Responses."""
+ __attrs__ = [
+ 'session', 'adapter_args', 'orig_request', 'response_chain']
+
+ def __init__(self, request, session, adapter_args):
+ self.session = session
+ self.adapter_args = adapter_args
+ self.orig_request = request
+ self.response_chain = []
+
+ @classmethod
+ def _compat_from_resolve_redirects(cls, req, resp, session, adapter_args):
@sigmavirus24

sigmavirus24 Feb 27, 2014

Owner

Classmethods are inherently public. Prefixing this with an _ is ugly and unnecessary. That aside, why is this even necessary? Why is __init__ not allowed to handle this? It could look like:

def __init__(self, request, response, session, adapter_args):
    self.session = session
    self.adapter_args = adapter_args
    self.original_request = request
    self.response_chain = [response]

@sigmavirus24 sigmavirus24 commented on the diff Feb 27, 2014

requests/sessions.py
- url = resp.headers['location']
- method = req.method
+ resp = self.session._send_internal(prepared_request, self.adapter_args)
@sigmavirus24

sigmavirus24 Feb 27, 2014

Owner

You seem insistent on declaring private methods and then using them as if they're public methods. Private methods are implementation details. You should only use the public API when not working with methods on the instance.

@sigmavirus24 sigmavirus24 commented on the diff Feb 27, 2014

requests/sessions.py
- # .netrc might have more auth for us.
- new_auth = get_netrc_auth(url) if self.trust_env else None
- if new_auth is not None:
- prepared_request.prepare_auth(new_auth)
+ extract_cookies_to_jar(prepared_request._cookies, prepared_request,
+ resp.raw)
+ prepared_request._cookies.update(self.session.cookies)
@sigmavirus24

sigmavirus24 Feb 27, 2014

Owner

Did you sneak a _cookies attribute onto the PreparedRequest object? There's no way that's sticking around. Sorry.

@sigmavirus24 sigmavirus24 commented on the diff Feb 27, 2014

requests/sessions.py
-
- # It's possible that users might accidentally send a Request object.
- # Guard against that specific failure case.
- if not isinstance(request, PreparedRequest):
- raise ValueError('You can only send PreparedRequests.')
-
- # Set up variables needed for resolve_redirects and dispatching of hooks
- allow_redirects = kwargs.pop('allow_redirects', True)
- stream = kwargs.get('stream')
- timeout = kwargs.get('timeout')
- verify = kwargs.get('verify')
- cert = kwargs.get('cert')
- proxies = kwargs.get('proxies')
- hooks = request.hooks
+ def _send_internal(self, request, adapter_args):
+ """Subroutine of `send`, also used by `RedirectIterator`.
@sigmavirus24

sigmavirus24 Feb 27, 2014

Owner

Like I said, any method called by something externally should be a public method, i.e., it should not be preceded by _.

@sigmavirus24 sigmavirus24 commented on the diff Feb 27, 2014

requests/sessions.py
+ in the original `send`, you probably need to pass them again here.
+
+ Provided primarily for backward compatibility; calling `send` with
+ `iter_redirects=True` is a friendlier API.
+ """
+
+ if req is None:
+ req = resp.request
+
+ # Set defaults as above.
+ kwargs.setdefault('stream', self.stream)
+ kwargs.setdefault('verify', self.verify)
+ kwargs.setdefault('cert', self.cert)
+ kwargs.setdefault('proxies', self.proxies)
+
+ return RedirectIterator._compat_from_resolve_redirects(req, resp,
@sigmavirus24

sigmavirus24 Feb 27, 2014

Owner

You're using an otherwise private API in a public manner. This is also far more verbose an unnecessary than simply creating an instance with the same parameters.

Owner

sigmavirus24 commented Feb 27, 2014

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.

Contributor

zackw commented Feb 27, 2014

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.

Owner

sigmavirus24 commented Feb 27, 2014

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.

Owner

sigmavirus24 commented Feb 28, 2014

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 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.

Owner

kennethreitz commented Mar 12, 2014

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 :)

Contributor

zackw commented Mar 12, 2014

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment