Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Issue arising from #1190 #1261

Closed
sigmavirus24 opened this Issue · 11 comments

3 participants

@sigmavirus24
Collaborator

The tests caught this purely by coincidence with #1259.

The issue is that we're now passing optional keyword arguments and typical hooks don't expect them causing exceptions to be raised.

In essence, this is a big breaking change and we will have to make it very clear to users that this is in the next version. requests/requests-kerberos depends on this but this is going to break a good amount of people's code.

Admittedly it only rears its head when we try and set the defaults via kwargs in Session.send. Again that's due to #1259/#1258 but @geoff-kruss is right in asserting that those defaults should be passed.

Assuming we keep the addition that causes this, we need a plan how to announce it.

@sigmavirus24
Collaborator

So I'm personally in favor of keeping the existing behaviour and trying to make it as clear as possible to users that the behaviour has been changed.

Also, I forgot to include @mkomitee in on this ticket.

@Lukasa
Collaborator

If I understand this correctly, we would have to at minimum move from 1.1.0 to 1.2 to accomodate this change. Is that right?

@sigmavirus24
Collaborator

Yeah this isn't a minor change. This is actually a more significant API change than I thought it would be when it was proposed.

@Lukasa
Collaborator

Hmm. On one hand, it's been quite a while since our last big API change. On the other, the last one caused some displeasure amongst some people. If we go ahead we definitely need to document it clearly.

@mkomitee

Frankly, I was surprised at how quickly my patch was accepted for this very reason. That said it's my opinion that the API without the change makes it impossible to do things that the user may expect if we're using an authentication handler which has to send requests. This was less of an issue pre 1.0 because authentication handlers had access to the session object if I recall correctly.

@sigmavirus24
Collaborator

Our last major change should actually make this less impactful. Fewer people are using hooks now than before because of the reduction of the set of hooks to a single one -- response. Most of the people using the response hook are using it for Authentication handlers. I know we have kerberos covered, and NTLM would be simple to fix (I'll be happy to send a PR to Ben), but I don't know about others.

Perhaps, you or Kenneth could make a blog post accompanying the release of 1.1.1/1.2 to announce this.

@mkomitee

Are there any other API changing patches pending release, or is this the only one?

@sigmavirus24
Collaborator

This is really the only one I can think of but I'll look over that tonight.

@mkomitee

What I did was the simplest way, but the whole

response.content
response.raw.release_conn()
response.connection.send(response.request)

rigamarole isn't that great IMO.

We could stick these user-supplied parameters on the request object, modify them (as well as headers if needed for authentication purposes) and then call a new method response.request.resend().

If we're okay with sticking these parameters on the request object itself, this may allow us to effect this change without altering the API.

@mkomitee

... and when I say modify them I mean expose them so that authentication handlers could modify them if they need.

@sigmavirus24
Collaborator

As part of pulling together the changelog tonight for #1233 I'm going to put together a blog post about this change. As such, I'm marking it closed because the "fix" has been decided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.