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

Improve hover results #452

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

HedgehogCode
Copy link

@HedgehogCode HedgehogCode commented Oct 6, 2023

This PR solves #444.

  • Use jedi Script.help instead of infer - Ensures that docstrings of the definition are used if possible instead of docstrings of the type
  • Show all possible signatures or types of the element hovered instead of just the first one that matches the name

I tried to describe the changes pretty well in the comments in the code. Note that there are two TODO(Review) comments that I would like feedback on.

Here are some examples of how the hover results change with this code change. The new results are highlighted with a green border.

hover_preview_1
hover_preview_2
hover_preview_3

@HedgehogCode HedgehogCode changed the title Improve hover results - solves #444 Improve hover results Oct 6, 2023
pylsp/plugins/hover.py Outdated Show resolved Hide resolved
pylsp/plugins/hover.py Outdated Show resolved Hide resolved
@PeterCardenas
Copy link

@tkrabel-db @HedgehogCode any way we can get this merged? looking forward to this change

@tkrabel-db
Copy link
Contributor

@PeterCardenas I'm afraid I have no rights to do so.
@ccordoba12 can you TAL?

@tbung
Copy link

tbung commented Jan 14, 2024

Thanks for your work on this! I am really looking forward for this to get merged.

I think there is still some room for improvement here, though. For variables with None in their union type or Optional type, the variable gets simply shown as NoneType. jedi-language-server is slightly better with showing the complete definition line in that regard.

Here is what python-lsp-server with this PR shows (without this PR python-lsp-server shows nothing at all in this situation):
Screenshot from 2024-01-14 03-09-21

And here is what jedi-language-server shows:
Screenshot from 2024-01-14 03-10-40

The real solution would be to correctly show a union type here. Since both lsps don't do that I assume this is an issue with jedi, I haven't check though.

PeterCardenas added a commit to PeterCardenas/python-lsp-server that referenced this pull request Jan 14, 2024
HedgehogCode and others added 3 commits February 7, 2024 09:16
- Use jedi Script.help instead of infer - Ensures that docstrings of the
  definition are used if possible instead of docstrings of the type
- Show all possible signatures or types of the element hovered instead
  of just the first one that matches the name
- Only show signatures for function and class types
- Also show types if there are definitons without signatures next to
  definitions with signatures
@HedgehogCode
Copy link
Author

Sorry for the late reply.

The example above showed NoneType() because jedi returns a signature for this definition. However, signatures are only interesting for "function" and "class" (BaseName#get_signatures). I changed the code to only show signatures for "function" and "class".
The new code shows Union[NoneType, int] on hover (this was already implemented - we just did not get there because of the signature).

I also changed the code to show the type in cases where we have a signature but this is not the only option. Example:

def b():
    return 1

if input() == "yes":
    b = 10

b

On hover over "b":

b()
Union[b, int]

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

Successfully merging this pull request may close these issues.

4 participants