add aiohttp support #11
Conversation
do3cc
left a comment
do3cc
left a comment
There was a problem hiding this comment.
After these minor changes, I'd merge this.
| # no aiohttp installed | ||
| return | ||
|
|
||
| from yarl import URL |
There was a problem hiding this comment.
What is yarl?
If it is from aiohttp, please put it in the try block
There was a problem hiding this comment.
Seems separate https://github.com/aio-libs/yarl
There was a problem hiding this comment.
it's a requirement of aiohttp
| try: | ||
| from aiohttp.client import ClientSession | ||
| except ImportError: | ||
| # no aiohttp installed |
There was a problem hiding this comment.
Add log message at level info here. If one day aiohttp changes import locations, this code will silently fail without giving a hint as to why it fails. better yet, if you want, don't catch for import errors at all but use pkg_resources to check if the aiohttp package is installed.
Then a changed import in aiohttp will trigger an exception which is even better than a log message.
|
|
||
| from yarl import URL | ||
|
|
||
| def patch(self, method, url, **kwargs): |
There was a problem hiding this comment.
I am fine with naming the patched method patch, but if you do it, please rename whitelisted above too. It should be consistent.
| return self.old_request(method, url, **kwargs) | ||
|
|
||
| if not getattr(ClientSession, 'blockage', False): | ||
| logger.debug('Monkey patching httplib') |
There was a problem hiding this comment.
You aren't monkeypatching httplib here. This debug message might be confusing.
| def patch(self, method, url, **kwargs): | ||
| yurl = URL(url) | ||
|
|
||
| if yurl.host not in whitelist: |
There was a problem hiding this comment.
Please add a log statement when an url passes the whitelist, as above.
3 participants
aiohttp doesn't use httplib but
asyncioTcp connection instead.aiohttpis basically an async version of httplib.This applies the patch to aiohttp without asking, because the user shouldn't be aware of httplib vs aiohttp, which IMHO is an internal detail of our approach.