Handle async do_complete in the debugger #520
Conversation
|
This is more to start a discussion than a "this must be merged". It's mostly to start some discussion also with upstream as we sort of disagree that having |
Carreau
force-pushed
the
do_comp_await
branch
from
2f295cd to
889df33
Compare
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.
Carreau
force-pushed
the
do_comp_await
branch
from
889df33 to
2e19c2c
Compare
|
Rebased. I think we agree upstream that things should mostly be async when possible. so +1 to merge this once tests are passing, and you agree., |
There was a problem hiding this comment.
Thanks @Carreau! A couple of small questions, the rest looks good to me.
| @@ -1435,6 +1439,7 @@ def test_django_settings(kernel): | |||
|
|
|||
| This is a regression test for issue spyder-ide/spyder#19516 | |||
| """ | |||
| import django | |||
There was a problem hiding this comment.
Why is this necessary now? It wasn't before.
There was a problem hiding this comment.
I do not remember.
I would say better error messages in pytest short summary:
- FAILED spyder_kernels/...::test_save_load_hdf5_files - assert '(None, "No m...ed \'h5py\'")' == "({'a': array...(4.5)}, None)"
+ FAILED spyder_kernels/...::test_save_load_hdf5_files - ModuleNotFoundError: No module named 'h5py'and
- FAILED spyder_kernels/...::test_django_settings - assert "'settings':" in '{}'
+ FAILED spyder_kernels/...::test_django_settings - ModuleNotFoundError: No module named 'django'There was a problem hiding this comment.
Ok, thanks for the clarification. Merging then.
ccordoba12
changed the title
A number of methods of ipykernel can optinally return
awaitable[T]instead of justT, this is the case fordo_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_completeis a coroutine function.This tries to handle it – and as of course
do_completercan 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.