Suggest to make implementations of some function always return awaitable #1295
Conversation
…ble. See discussion in ipython#1272; It is not deprecated, but being able to always know you can (and must) await should be simpler in the long run. Deprecating now is not the point, but I want to cover our bases, so that we are more confident later when and if we want to enforce those await. In particular many of those branches are not covered in our tests – and I don't even know wether they were ever taken; I changed some of the base methods to be async, but I'm happy to move those back to sync. A few other things use the `if awaitable(...):` pattern but are a bit more complicted, and some do not dates from 2021, so those will be dealt with separately.
Carreau
force-pushed
the
always-await
branch
from
bf4f57d to
0a69e5f
Compare
|
I'm going to revert making the default implementation awaitable later, but currently this breaks some downstream things in the way I was expecting as consumers don't check wether the return is an awaitable or not. |
A number of methods of ipykernel can optinally return `awaitable[T]` instead of just `T`, this is the case for `do_complete`. I think it's a mistake ; see ipython/ipykernel#1295 ; in particular because it's easy to forget / hard to properly type-check, and I'd like to make it mandatory in the long term to have await. Spyder seem to not handle the case where do_completer return an awaitable (or more partiularly is `do_complete` is a coroutine function. This tries to handle it – and as of course `do_completer` _can_ be async, all caller _must_ be async. So I try to do all the required updates. Note: I also add explict imports in some test, to get better error message in case those deps are not installed.
|
I don't object to using 7.0 to make all base class One thing I'll note is that allowing subclasses to implement methods as sync or async is one thing (what we do in ipykernel), whereas changing a base class implementation to be async or not (has happened some places in 7.0 I think, this PR makes it more consistent) is definitely a straightforwardly breaking change. It is not the case that the current API where subclasses may define methods as async means that the base class can switch between sync and async without it being considered a major breaking change. Allowing multiple implementations of these methods is really the point of the Kernel class, but so is a stable base implementation. |
|
I agree, right now I'm changing to
I don't like breaking things without warnings either; But I think that it's (too) easy to implement a subclass, or using a class without asking wether the methods could be async if the default implementation is sync; and I'd like to curb the complexity of navigating and understanding those codebase. I don't touch them that often; but we had a couple of client request, and getting back into that for fixes or advices with all the conditional options makes it IMHO more challenging than it should be. |
See discussion in #1272; It is not deprecated, but being able to always know you can (and must) await should be simpler in the long run.
Deprecating now is not the point, but I want to cover our bases, so that we are more confident later when and if we want to enforce those await.
In particular many of those branches are not covered in our tests – and I don't even know wether they were ever taken;
I changed some of the base methods to be async, but I'm happy to move those back to sync.
A few other things use the
if awaitable(...):pattern but are a bit more complicted, and some do not dates from 2021, so those will be dealt with separately.