Skip to content
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

docstrings/type hints for .encode() options #2884

Closed
mattijn opened this issue Feb 12, 2023 · 6 comments · Fixed by #2920
Closed

docstrings/type hints for .encode() options #2884

mattijn opened this issue Feb 12, 2023 · 6 comments · Fixed by #2920

Comments

@mattijn
Copy link
Contributor

mattijn commented Feb 12, 2023

On the wishlist. I really like the merge of this PR #2846 by @binste. It really makes it possible to explore the options while building an altair specification.

Maybe it is not directly related, bit I wished that the possible options within the .encode() were similarly shown as well. Currently the possible encoding channels are presented as *args, **kwargs:
image

@mattijn
Copy link
Contributor Author

mattijn commented Feb 19, 2023

Is this something you can push a bit forward @binste?

@binste
Copy link
Contributor

binste commented Feb 20, 2023

I like the idea. Maybe I can add the channels as explicitly listed arguments to encode somehow during code generation. I'll look at this later in the week.

@joelostblom
Copy link
Contributor

In JupyterLab I can see the competion when clicking shift + tab, but due to ipython/ipykernel#1065 I have to assign to an intermediate object first:

image

@binste
Copy link
Contributor

binste commented Feb 21, 2023

My current understanding is that many methods and classes in api.py and mixins.py use the utils.use_signature decorator to overwrite their signature (by setting __wrapped__) and docstring (setting __doc__) based on another class. For encoding the code looks like this:

@utils.use_signature(core.FacetedEncoding)
def encode(self: _TEncodingMixin, *args: P.args, **kwargs: P.kwargs) -> _TEncodingMixin:

This allows the completion in Jupyter (Jedi) to show the signature as shown in the screenshot of @joelostblom and even the relevant docstring. However, it does not give autocompletion support for keywords, e.g. if you start typing encoding(xO you probably don't get a prompt for xOffset.

For autocompletion engines such as Pylance which is used in VS Code, this approach does not work. We already briefly discussed this in #2846. Reason is that Pylance uses a static type checker to infer types, signatures, docstrings, etc. The code is not executed and use_signature can therefore not modify the relevant attributes which are only available at runtime. Also see microsoft/pylance-release#3958 (comment) where this is confirmed.

Potential approaches

  • ditch use_signature and auto-generate the content instead. But that might be a bigger rewrite to how the code is generated as e.g. api.py is currently not modified by the code generation.
  • There are many related issues in the pylance GitHub repo which I'm currently going through and I'm hoping to find a way to solve this with type annotations, i.e. that there is a way to annotate the function signature of encode to be the same as core.FacetEncoding.__init__. Not sure if this will also work for docstrings.
  • Auto-Generate type annotations in stub files (.pyi). Much less intrusive then the first approach. We could iterate through all objects and methods and see which are wrapped with use_signature. For those, we could write out the docstring and signature into stub files. I have never used this before, not sure if it works, especially for docstrings.

I think getting this right would be a big usability increase for VS Code, PyCharm, and other similar editors and language servers as it not only affects encoding but many of the Chart methods and _PropertySetter

@ChristopherDavisUCI
Copy link
Contributor

@binste @joelostblom Is there an option to use this same sort of syntax for encode?
https://github.com/altair-viz/altair/blob/ef8ff946bf610c19ccbecfbbd6b624004c25fb32/altair/vegalite/v5/schema/channels.py#L372-L374
as in #2795? (I mostly tested that in JupyterLab, so am not even totally sure that those type annotations were working in VS Code.)

@binste
Copy link
Contributor

binste commented Feb 22, 2023

Thanks for linking the relevant PR and code @ChristopherDavisUCI! Overload works great for autocompletion engines such as the one used in VS Code. Indeed, this might be a good approach. Especially, if we could define those overload signatures in separate stub files to not have to modify api.py as described above. I aim to have a PR ready by Friday in the hope that it can still make it into rc1.

Update: ParamSpec works great for this and is easy to add to the existing code base as it can be used on use_signature. Not sure yet how to get the docstrings to show up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants