asyncio API and discord mirror (seeking feedback)

[dehnert]

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:

• As you can see, it hasn't been properly rebased. I ported a big chunk of it in one go, and then made some tweaks (eg, the iterator version of 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?
• Ditto, but for the Discord mirror -- I feel a bit better with the current "first the actual mirror, then the docs and feedback" situation, but I could combine things. I may also add some more features soon-ish, and it'd be good to know if I should squash those in too (my inclination would be "no, leave them separate").
• I'm not super clear on what the typing should be like for async functions -- is async def call_endpoint(...) -> Awaitable[Dict[str, Any]]: correct? The Awaitable feels sorta unwieldy and feels a little like it should be inferable from the async, but maybe that's the right thing?
• mypy doesn't like async RandomExponentialBackoff because fail changes from a function returning None, to a coroutine returning None. It seems to work, and seems a pity to duplicate CountingBackoff, but I'm not sure there's a good way to appease the type-checker (or if there's a subtle error).
• Does the existing API client have tests? The async API is very similar, so it's possible that any existing tests could be trivially or programmatically converted to run against the async API.
• Relatedly, most of the async API is basically "take the sync API, add async before def, and await after return". 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...
• I pretty directly ported 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.

[timabbott]

@andersk would you be up for doing the review on this?

[andersk]

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 awaited (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

  • Raise exceptions for {"result": "error"} in zulip apis. #672
  • zulip: Reraise exceptions in do_api_query.  #735

• 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.

[merlinz01]

I would be interested in an async version of the client to improve overall performance in an async server environment.

Have you thought about generating both sync and async clients based on the OpenAPI specification? Although I don't have experience with doing so, I have heard it recommended as a best practice.

[timabbott]

@dehnert is this ready for review? You don't appear to have posted a comment when pushing to this branch after @andersk's comments, which results in GitHub not having notified us about your push. In any case, it'd be great to understand whether you believe you've addressed the feedback.

[andersk]

No, we were notified about the push. It added two Discord-related commits and addressed none of my comments.