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

extract_cookies_to_jar requires a response with _original_response #1534

Closed
dstufft opened this issue Aug 16, 2013 · 3 comments · Fixed by #1535
Closed

extract_cookies_to_jar requires a response with _original_response #1534

dstufft opened this issue Aug 16, 2013 · 3 comments · Fixed by #1535

Comments

@dstufft
Copy link
Contributor

dstufft commented Aug 16, 2013

While attempting to make a LocalFSAdapter() which handles file:// urls I discovered that the response objects need to have an _original_response attribute or it bombs out at https://github.com/kennethreitz/requests/blob/master/requests/cookies.py#L113.

@sigmavirus24
Copy link
Contributor

This probably doesn't help but a few things:

  • Requests is ill-suited for the file:// scheme.
  • The _original_response object can be faked in a couple ways but only one way if you're interested in python 2 and 3 support. If the latter is your purpose then checkout sigmavirus24/betamax. I'm using the latter method there. If you have questions, feel free to ask me outside of this message.

I also wouldn't be opposed to documenting the method, but for the most part, I don't see this being a bug. Maybe @Lukasa or @kennethreitz

@dstufft
Copy link
Contributor Author

dstufft commented Aug 17, 2013

Heh, @kennethreitz is the one who told me to file it :) I actually have it mocked and it's pretty janky. Simple fix for upstream though and the file:// url is actually working pretty well, requests seems to handle it fine. The only "gotchas" are this one which requires a pretty janky hack to get around, and an error because file:// urls don't have a hostname but another small hack can fix that too.

FWIW: dstufft/pip@aba6f37

@sigmavirus24
Copy link
Contributor

The fix is simple without a doubt and since it seems @kennethreitz seems okay with it, I'll put together a PR with it.

That said, the best workaround is not that janky frankly but you're right in that it shouldn't be necessary. My project needs it though :/

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants