Moved callback execution to separate methods. #490
Conversation
|
Looks like the unit tests weren't too happy with it, but I really do like this concept and I think it's absolutely a move in the right direction |
nio/client/base_client.py
Outdated
| def execute(self, event, room: Optional[MatrixRoom] = None) -> Optional[Awaitable]: | ||
| if self.filter is None or isinstance(event, self.filter): | ||
| result = self.func(room, event) if room else self.func(event) | ||
| if inspect.isawaitable(result): | ||
| await result | ||
| if room: | ||
| return self.func(room, event) | ||
| else: | ||
| return self.func(event) | ||
|
|
||
| async def async_execute(self, event, room: Optional[MatrixRoom] = None) -> None: | ||
| result = self.execute(event, room) | ||
| if inspect.isawaitable(result): | ||
| await result |
There was a problem hiding this comment.
In practice, both the sync and async clients need to attempt to invoke a callback as either sync or async; someone can reasonably pass a sync (and hopefully, nonblocking) callback to an async client and it ought to work
There was a problem hiding this comment.
There wasn't one method for each before. In stead, in places where it was done solely synchronously, it was hard-coded to loop and do the if-statements. All this does is to put that into a method to avoid repetition. And to further avoid repetition, the asynchronous method just calls the synchronous one; if an awaitable is returned, await it.
Everything should work exactly as before. Are you suggesting the synchronous method should have an extra step added for if the function is asynchronous? For example:
def execute(self, event, room: Optional[MatrixRoom] = None) -> Optional[Awaitable]:
if self.filter is None or isinstance(event, self.filter):
if room:
result = self.func(room, event)
else:
result = self.func(event)
if inspect.isawaitable(result):
asyncio.run(result)
nio/client/base_client.py
Outdated
| if inspect.isawaitable(result): | ||
| asyncio.run(result) |
There was a problem hiding this comment.
It seems like that the main motivation to have a separate sync_execute and async_execute is right here: the ability to support async callbacks in a sync client.
Considering the extra complexity it introduces, and considering that it involves implicitly supporting/adding new features to a soft-deprecated usecase (using matrix-nio without async) I'm tempted to say that we might not care to include this extra complexity, and stick to the original implementation of execute here.
I don't generally expect people who know how to write async def callback functions to still be using the sync client, as well.
Simultaneously, I don't think this is a bad change and it's fairly straightforward. So I'm more tempted to keep it.
nio/client/base_client.py
Outdated
| # Checks the filter and executes the function once. | ||
| # sync_execute and async_execute will each determine | ||
| # how to run an awaitable if one is returned. |
There was a problem hiding this comment.
Use docstring formatting
nio/client/base_client.py
Outdated
| if inspect.isawaitable(result): | ||
| asyncio.run(result) |
There was a problem hiding this comment.
I also just noticed it as I was making the doc change: asyncio.run has a slightly different requirement than simply taking an awaitable
| if inspect.isawaitable(result): | |
| asyncio.run(result) | |
| if inspect.iscoroutine(result): | |
| asyncio.run(result) |
I've moved the execution loops of callbacks to separate methods. This is so that the behaviour can be containedly redefined without needing to hard-rewrite all surrounding logic. As it is now, subclassing the client and adding functionality on running the callbacks would entail needing to keep an eye out for any changes to these whole methods upstream, lest an update be missed and either break it or simply not benefit from patches or upgrades to the flow.