Replace urllib by requests in contentproviders #993
Conversation
|
Any chance to have this PR being merged? Thanks |
There was a problem hiding this comment.
Sorry for the super slow response. I ended up taking some time off from maintaining duties to recharge.
I left a few nit picks but otherwise this looks like a huge effort and as far as I can tell it doesn't change anything in the tests besides switching the mock. This is a good thing :)
repo2docker/contentproviders/doi.py
Outdated
There was a problem hiding this comment.
Same comment as further down: useful for the average user on a day to day basis or more of a debug help?
There was a problem hiding this comment.
Good question. Previsouly, this fetchfile() method did log (as generated strings):
- the directory creation ("Creating [...]"),
- the saving of the downloaded zip file in
dst_fname("Fetching [...]") - the zip file extraction ("Extracting [...]")
- the list of extracted files ("Extracted [...]")
In my version of the code, the only thing logged in addition to these is the "Requesting [...]" line in which I display the actually retrieved URL (after doi resolution) I though would be useful in a log.
requests is globally simpler to use, and more and more people are more familiar with this later than urllib.
|
Congrats on your first merged pull request in this project! 🎉 |
2 participants
requests is globally simpler to use, and more and more people
are more familiar with this later than urllib.
See #988 for a discussion on why this is needed.