Allow client.on_session_change to Use Both Coroutine and Regular Functions

[comfuture]

The requirement for the callback function client.on_session_change to be a coroutine in AsyncClient causes inconvenience. In many cases, detecting session changes and storing them in an arbitrary repository can be sufficiently fulfilled without being async.

When using AsyncClient, it is easy to assume that all related methods and functions are coroutines. However, there are exceptions. The client.export_session_string() method is always a blocking function.

Expected:

It would be beneficial if the client.on_session_change callback function could be inspected internally to allow the use of both coroutine and regular blocking functions.

[MarshalX]

I understand that could be confusing at the first look, but you will got clarify right after looks at method/function signature. Just right after hovering on the method in IDE.

About export_session_string: I do not see any particular reason to mark methods with async if they do not performs I/O operations inside. Passing it to the event loop will be more negative.

Speaking of on_session_change it support both sync and async depending of the client. And there is a huge reason to ake async available. Because I do not know developers needs. Their needs could be to store session string somewhere in DB. And writing to DB is I/O bound operation. It will be much harder if the developer wants to do proper async write inside on_session_change if it will be sync. I understand that devs could do sync operation and it will be performed in the same way as I described export_session_string. We will never know how they will use it.

Combining 2 types of callbacks is a real pain to make it work perfectly with static type checkers (even basic one in IDE). Mixing both interfaces also is not simple task in Python... But this particular case probably could be easily done with one additional "if" statement and blocking call of the handlers. And pray that dev will not put something huge into the handler which will stop the world.

tl;dr sync/async is always a choice depending on the performed operations inside and possible usage; not "let's always use async because it is AsyncClient"; it is hard to maintain both sync and async. adding more logic around async to allow sync sometimes will be even harder (I am the only one maintainer)...

[comfuture]

I agree with your intention, and I apologize for any confusion caused. My mention of export_session_string was merely an example to illustrate that not every method in AsyncClient needs to be asynchronous.

In other words, my suggestion was regarding whether there is a need for the on_session_change callback to be implemented differently in Client and AsyncClient.

Looking at the implementation, in AsyncSessionDispatchMixin, the on_session_change method is overridden to accept an AsyncSessionChangeCallback. How about not overriding it and keeping the original method that accepts a SessionChangeCallback, then branching internally with inspect.iscoroutinefunction(callback)?

Since the callback function returns None (as it does not need to return anything), there is little need to annotate this with t.Coroutine[t.Any, t.Any, None] just for typing purposes. Modern IDEs can correctly infer this with just the actual return value (None) for async functions.

Thank you for your precise type specifications and your excellent efforts in maintaining this module. While I still lack extensive knowledge in typing, I hope to contribute someday.