-
Notifications
You must be signed in to change notification settings - Fork 793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve autocompletion for arguments to objects wrapped with use_signature #2920
Conversation
Nice that you've introduced the typing_extension, it mades the required type hints easier to read. I wish I have the time to do one big review, but for now just add some comments when I find some time to test.
|
Thanks! I can replicate the issue in Python 3.9, didn't notice it during development as I was on 3.11. I thought the With this adjustment, method chaining and keyword completion works for me in both Python 3.9 and 3.11. Could you try again? Maybe it also helps the language server in Jupyter. |
Tests work locally (Python 3.11) but fail in workflow due to the removal of VL |
Will try to get #2871 in later today |
Co-authored-by: Mattijn van Hoek <mattijn@gmail.com>
I can replicate this and it feels like a bug in python-lsp-server or whichever package is responsible for doing the static code analysis. The below is a minimal reproducible example which I'm running on Python 3.9 with newest Jupyterlab, Jupyterlsp, and python-lsp-server[all]: from typing import Callable
from typing_extensions import Self
def use_signature(Obj):
def decorate(f: Callable):
return f
return decorate
class SignatureClass:
def __init__(self, arg1, arg2):
self.arg1 = arg1
class MainClass:
@use_signature(SignatureClass)
def returns_self(self, *args, **kwargs) -> Self:
return self
def another_method(self):
pass This replicates the When removing I currently can't find a reason why annotating a function with |
I've the same feeling that this a limitation of jupyter-lsp and not intended. I agree that we should raise an issue at the relevant repo and merge this in if you don't change your mind after some sleep. |
Opened this issue in the jedi repo. Great to see 5.6.1 in master! Merged it into this branch and reran the schema generation. Changes still work in VS Code for me. If tests pass, it's good to go from my side. |
All tests pass and since:
Can merge this in and the jupyter ecosystem can benefit of this PR once jedi supports the typehint |
Resolves #2884, see that issue for more context. All methods and functions which are wrapped with
use_signature
profit from this PR. The PR can be split into three main changes which were all necessary to make this work.Full function signature:
Keyword completion (which still does not work for me in Jupyter, just VS Code):
Long method-chaining still works:
Function signature and docstrings still show up in Jupyter as before.
I wasn't able to also get docstrings working in VS Code. They still require inspection at runtime and therefore only work in Jupyter. I don't think this is too bad, I find the function signature and autocompletion on keywords much more important as I usually tend to look up this information in the documentation anyway which is better formatted.
Type hints on
utils.use_signature
Using the example:
the new type hints on
use_signature
tell a type checker thatencode
has the same parameters ascore.FacetedEncoding
but keeps its original return typeSelf
which is whatever the type ofself
is. This is the main part. The other changes are necessary to make sure this also works with method chaining.As
typing.ParamSpec
was introduced in Python 3.10 I addedtyping_extensions
as a new requirement to Altair. Already briefly discussed this with @mattijn here. It's a very stable dependency and I think now it's worth adding it.Switching from self-bound
TypeVar
s toSelf
Now that we have
typing_extensions
we can switch toSelf
. See linked comment above for the discussion. This is also necessary as the new type hints onuse_signature
did not work with the self-boundTypeVar
s.Move
utils.use_signature
from class to__init__
definitionsIn the instances where
use_signature
was wrapping a class I had to move it to wrapping__init__
instead as else method chaining such asalt.Chart().repeat()...
did not work. Reason is that e.g.repeat
should showFacetChart
as return type but without this change it showsAny
as the type hints onuse_signature
no longer work when a class is wrapped.This should not have any averse effects on the functionality of Altair. Only change I found is that in the documentation, the docstrings are now included in
__init__
instead of the top-level class which I don't think is bad as Python allows to document a class either on the top-level class or in__init__
and IDEs usually treat this as the same. Old:New:
For consistency, I could move the manually defined docstrings on classes such as
alt.Chart
also to__init__
. Let me know if you'd prefer this in the same PR or if we want to postpone/don't do this change and merge the PR as is. I don't think this alignment is very important but a nice-to-have.