Add an asyncio version of the HttpApi class #168
Conversation
|
This is a version of the code in this repo: https://github.com/opsdroid/connector-matrix (that I am the author of). |
matrix_client/_api_asyncio.py
Outdated
| loop.run_until_complete(main()) | ||
| """ | ||
|
|
||
| def __init__(self, base_url, client_session, token=None): |
There was a problem hiding this comment.
I think being able to pass in a client session is definitely a nice thing, but
in the interest of simplicity, is there any way we could have a default
client_session if none is provided? This would require changing the sdk to
have an optional dependency on aiohttp; are optional dependencies a thing that
exists in python-land?
There was a problem hiding this comment.
The thing is the recommended way to create the session is with a context manager. I am not the sure of the best way to handle closing the session if it is created in the constructor of the class.
There was a problem hiding this comment.
Could you link me to some docs on the "sessions" you're talking about? Having a hard time understanding what cleanup is necessary. EDIT: nevermind I found it.
There was a problem hiding this comment.
The first example on the first page for using aiohttp client uses ClientSession outside of a context manager, so I think we're okay on that front. Bigger question to me is if we want to tie ourselves to aiohttp as the default or if asks is the better option with the way the ecosystem seems to be leaning away from asyncio and towards curio/trio.
There was a problem hiding this comment.
Actually I've flip-flopped on this again in the past 10 minutes. We should write the api class as generically as possible so that wrappers at the same abstraction level as MatrixClient can use whatever their preferred coroutine library is with this easily, so given that I think you can use asks with this as is, it's probably pretty good right now..
|
I need some tests on this code before I can merge it. I know that's a |
matrix_client/_api_asyncio.py
Outdated
| method, | ||
| path, | ||
| content=None, | ||
| query_params={}, |
There was a problem hiding this comment.
IIRC, this instantiates a single empty dictionary that's reused across all calls
to this method. Probably not what we really want.
There was a problem hiding this comment.
Indeed, it was like this in _send in the API class as well. I copy and paste all bugs.
matrix_client/_api_asyncio.py
Outdated
| content=None, | ||
| query_params={}, | ||
| headers={}, | ||
| api_path="/_matrix/client/r0"): |
There was a problem hiding this comment.
This should pull from the same place as MatrixHttpApi gets its default url
path so that we can update that all at once in a single location.
matrix_client/_api_asyncio.py
Outdated
|
|
||
| if "Content-Type" not in headers: | ||
| headers["Content-Type"] = "application/json" | ||
|
|
There was a problem hiding this comment.
A lot of this is copy-pasted from MatrixHttpApi. Please move common
functionality out to a helper function rather than duplicating code.
matrix_client/_api_asyncio.py
Outdated
| headers["Content-Type"] = "application/json" | ||
|
|
||
| if self.token: | ||
| query_params["access_token"] = self.token |
There was a problem hiding this comment.
This right here is why we need to make sure we're not reusing the same
dictionary between requests.
matrix_client/api_asyncio.py
Outdated
| import sys | ||
|
|
||
| if sys.version_info < (3, 5): | ||
| raise ValueError("The asyncio version of the api is only supported on Python 3.5+") |
There was a problem hiding this comment.
I'm not sure ValueError is the correct error here, but I'm not sure what the
ideal would be anyways. Figuring it out might not be a worthwhile endeavor given
how often this code-path should be run (never).
|
I am a little confused by the coveralls setup, it seems we are running it on all the jobs, but coveralls only takes the first report? Anyway the coverage will be screwy on anything <3.5 |
|
If the new asyncio tests were included in the coveralls report it would be green. |
This adds a Python 3.5+ async def / await version of the
MatrixHttpAPIclass. It also adds an import shim to give a nice (i.e. not syntax) error if a user tries to import it on <3.5.I have asked a couple of people knowledgeable in such things and I expect no issues with having sections of the package to have different Python version requirements than others, as long as it is not imported there will be no trouble.