-
Notifications
You must be signed in to change notification settings - Fork 209
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
Hover should not show docstring of inferred type but help for hover position #444
Comments
Thank you, this was bugging me for a while too. If the performance-wise there is no penalty in using |
@HedgehogCode, could you post a screenshot of what you see when using |
Thanks @HedgehogCode! I agree that this would be an improvement, but it'd be better if we'd map the docstring returned by
which I think is more helpful. Actually, I have a branch that implements a similar improvement in Spyder itself. We plan to have that in Spyder 6, but I could move that code to the server instead. |
I agree, that this is more useful if no docstring is attached to the variable. However, if someone took the time to write a docstring for the variable I think it should be shown to the user. Pyright for example shows both (result from a "textDocument/hover" request): {
"contents": {
"kind": "markdown",
"value": "```python\n(variable) bar: Literal[10]\n```\n---\nA variable named bar"
},
"range": [...]
} Note that the variable docstrings are not a language feature but the format is generally accepted (Jedi, sphinx, Pyright, and probably more tools support it). |
Sure, that's a good point too. But I don't know how to do that and show the variable type with Jedi unless (I guess) users also add a type when declaring the variable. |
Yes. Showing the value of a variable instead of the docstring of the built-in type is much more useful. However, I think we should also show the docstring of the variable itself if it has one (like in my initial example). I also started working in this direction here but I want to do some more testing and provide some examples for hovering different elements. |
If this is done for builtins only, then it's fine. I say it because trying to get values of objects using |
I don't do that via |
Note that I created the PR #452 to solve this ticket. The PR shows some screenshots of new hover results. The proposed changes do not include the approach by @SigmaTel71 because I came to the conclusion that the definition of a statement should not be shown in the hover result. The language server protocol has a separate method to find the definition of an object: "textDocument/definition". However, I am open to discuss that. |
I believe this reasoning is incorrect, the intent of the definition request is not to present it in a hover but to enable go to/jump behaviour. It only returns a location object which does not have a field for text representation of the definition. |
I started implementing variable definition lookup on hover because this is what pyright does, but I don't want to use pyright just for that, given the fact that I moved from VSCode to Sublime Text for better workflow experience when working on Python code. |
I see. That makes sense. So the question is more, if we really want to show a definition lookup in the hover result.
I could be wrong, but I think the pyright results are types rather than definition lookups. I put some type inference into my PR but Jedi is less strict (does not use the type If we decide on showing a definition lookup we should answer the following questions:
I am open to suggestions for enhancing my PR. |
For me using
From what I see in pyright, the definition is shown when the value is literal, no matter if it's a constant or variable.
I think it will be effective enough to show it in order like 'type, value (if applicable), docstring (if exists)'. |
Try the following script:
When using auto-complete while typing "bar" the documentation of the variable is resolved correctly. However, if I hover over "bar" it shows the documentation of
int()
which is not helpful at all.The current implementation of hover uses Script.infer. Using Script.help gives the expected hover content.
The text was updated successfully, but these errors were encountered: