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

Cleanup #6

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Cleanup #6

wants to merge 3 commits into from

Conversation

gazpachoking
Copy link

This cleans up a few things:

  • Counter variables have been changed to increment with the loop
  • Requests automatically handles redirects for get requests (unless you tell it not to), so I removed that code
  • Fixed so that downloads actually stream to disk instead of getting entirely loaded into memory first

@portablejim
Copy link
Owner

I think I debugged the code and added the manual redirects in because they were needed. (Then again, I could be wrong).

As for the steaming to disk, I don't like that. Even though it would be nice, having the whole file in memory avoids the ability for partial downloads to be saved to disk and therefore avoids the integrity checks (did I actually get all the file properly if last connection was cut/interrupted part way through?) that is hard to get with the API. (Well, at least that is what I am trying to do. If I messed it up, tell me)

@gazpachoking
Copy link
Author

I think I debugged the code and added the manual redirects in because they were needed. (Then again, I could be wrong).

The docs say this:

True if this Response is a well-formed HTTP redirect that could have been processed automatically (by Session.resolve_redirects()).

So, it's a requests bug if the redirects aren't being resolved, and should be addressed there.

As for the streaming, I think it's much nicer. If you are worried about incomplete downloads we could just have it save to somefile.tmp and move it to real name once we are sure it completed successfully. If we aren't going to do that, I think the stream= kwarg should be removed, as it's misleading and does nothing the way it's currently used.

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

Successfully merging this pull request may close these issues.

2 participants