Open dehnert opened 2 years ago
@andersk would you be up for doing the review on this?
Not sure if I’ll have time for anything like a full review soon, but some quick thoughts:
An async def
should not be annotated -> Awaitable[…]
. The purpose of Awaitable
is for typing non-async
definitions that will be await
ed (documentation).
async def a() -> int:
return 1
b: Callable[[], Awaitable[int]] = a
def c() -> Awaitable[int]:
return a()
The new aiohttp
requirement needs to be declared.
Let’s not propagate the mistake from the sync API of returning server errors as if they were normal values; those should raise exceptions. See
asynch
is a weird name. I know this is a workaround for async
being a keyword, but consider aio
instead.
I’m not excited about the maintenance cost of duplicating all our functions between the sync and async APIs. We should think about whether we want to deprecate the sync API and/or replace it with a wrapper around the async API.
This PR adds support for an asyncio Zulip API (#483) and a discord mirroring script using it. It's definitely not ready to merge, but I think it'd be useful to have somebody look it over and provide some more feedback before I get it mergeable. The async client is mostly a pretty direct port of the original client, though obviously some of the lower-level functions had more changes.
Some things I'd like guidance on / places that seem suboptimal that maybe there's a better solution for:
call_on_each_event
,RandomExponentialBackoff
) in separate commits, and then various later commits cleaning up lint errors. What degree of commit splitting seems useful? Presumably the lint fixup commits should be merged to be earlier, but is it useful to leave any of the early commits separate, or should I just have one "add an async API" commit?async def call_endpoint(...) -> Awaitable[Dict[str, Any]]:
correct? TheAwaitable
feels sorta unwieldy and feels a little like it should be inferable from theasync
, but maybe that's the right thing?mypy
doesn't like async RandomExponentialBackoff becausefail
changes from a function returningNone
, to a coroutine returningNone
. It seems to work, and seems a pity to duplicateCountingBackoff
, but I'm not sure there's a good way to appease the type-checker (or if there's a subtle error).async
beforedef
, andawait
afterreturn
". It's conceivable there's a good way to automate this? I didn't bother to, but I also didn't bother converting all the methods...do_api_query
, which has a lot of error handling code. I tried to update it appropriately, but haven't tested all the ways things could fail. It's possible it makes more sense to delete a bunch of the error handling, and re-add it as people discover issues, rather than have untested error handling that I don't especially understand? Dunno.