From 1652f80a7ffa0cefc46fa425f9a4344ab243705f Mon Sep 17 00:00:00 2001 From: Benjamin Wilhelm Date: Fri, 6 Oct 2023 10:08:10 +0200 Subject: [PATCH 1/3] Improve hover results - 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 --- pylsp/plugins/hover.py | 90 ++++++++++++++++++------- test/plugins/test_hover.py | 134 ++++++++++++++++++++++++++++++++----- 2 files changed, 180 insertions(+), 44 deletions(-) diff --git a/pylsp/plugins/hover.py b/pylsp/plugins/hover.py index ca69d1b3..ad4f2a56 100644 --- a/pylsp/plugins/hover.py +++ b/pylsp/plugins/hover.py @@ -8,43 +8,81 @@ log = logging.getLogger(__name__) +def _find_docstring(definitions): + if len(definitions) != 1: + # Either no definitions or multiple definitions + # If we have multiple definitions the element can be multiple things and we + # do not know which one + + # TODO(Review) + # We could also concatenate all docstrings we find in the definitions + # I am agains this because + # - If just one definition has a docstring, it gives a false impression of the hover element + # - If multiple definitions have a docstring, the user will probably not relize + # that he can scroll to see the other options + return "" + + # The single true definition + definition = definitions[0] + docstring = definition.docstring( + raw=True + ) # raw docstring returns only doc, without signature + if docstring != "": + return docstring + + # If the definition has no docstring, try to infer the type + types = definition.infer() + + if len(types) != 1: + # If we have multiple types the element can be multiple things and we + # do not know which one + return "" + + # Use the docstring of the single true type (possibly empty) + return types[0].docstring(raw=True) + + +def _find_signatures(definitions, word): + # Get the signatures of all definitions + signatures = [ + signature.to_string() + for definition in definitions + for signature in definition.get_signatures() + if signature.type not in ["module"] + ] + + if len(signatures) != 0: + return signatures + + # If we did not find a signature, infer the possible types of all definitions + types = [ + t.name + for d in sorted(definitions, key=lambda d: d.line) + for t in sorted(d.infer(), key=lambda t: t.line) + ] + if len(types) == 1: + return [types[0]] + elif len(types) > 1: + return [f"Union[{', '.join(types)}]"] + + @hookimpl def pylsp_hover(config, document, position): code_position = _utils.position_to_jedi_linecolumn(document, position) - definitions = document.jedi_script(use_document_path=True).infer(**code_position) - word = document.word_at_position(position) - # Find first exact matching definition - definition = next((x for x in definitions if x.name == word), None) - - # Ensure a definition is used if only one is available - # even if the word doesn't match. An example of this case is 'np' - # where 'numpy' doesn't match with 'np'. Same for NumPy ufuncs - if len(definitions) == 1: - definition = definitions[0] - - if not definition: - return {"contents": ""} + # TODO(Review) + # We could also use Script.help here. It would not resolve keywords + definitions = document.jedi_script(use_document_path=True).help(**code_position) + word = document.word_at_position(position) hover_capabilities = config.capabilities.get("textDocument", {}).get("hover", {}) supported_markup_kinds = hover_capabilities.get("contentFormat", ["markdown"]) preferred_markup_kind = _utils.choose_markup_kind(supported_markup_kinds) - # Find first exact matching signature - signature = next( - ( - x.to_string() - for x in definition.get_signatures() - if (x.name == word and x.type not in ["module"]) - ), - "", - ) - return { "contents": _utils.format_docstring( - # raw docstring returns only doc, without signature - definition.docstring(raw=True), + _find_docstring(definitions), preferred_markup_kind, - signatures=[signature] if signature else None, + signatures=_find_signatures(definitions, word), ) } diff --git a/test/plugins/test_hover.py b/test/plugins/test_hover.py index a65f92dc..399f015f 100644 --- a/test/plugins/test_hover.py +++ b/test/plugins/test_hover.py @@ -9,10 +9,44 @@ DOC_URI = uris.from_fs_path(__file__) DOC = """ +from random import randint +from typing import overload -def main(): - \"\"\"hello world\"\"\" +class A: + \"\"\"Docstring for class A\"\"\" + + b = 42 + \"\"\"Docstring for the class property A.b\"\"\" + + def foo(self): + \"\"\"Docstring for A.foo\"\"\" + pass + +if randint(0, 1) == 0: + int_or_string_value = 10 +else: + int_or_string_value = "10" + +@overload +def overload_function(s: int) -> int: + ... + +@overload +def overload_function(s: str) -> str: + ... + +def overload_function(s): + \"\"\"Docstring of overload function\"\"\" pass + +int_value = 10 +string_value = "foo" +instance_of_a = A() +copy_of_class_a = A +copy_of_property_b = A.b +int_or_string_value +overload_function + """ NUMPY_DOC = """ @@ -23,6 +57,83 @@ def main(): """ +def _hover_result_in_doc(workspace, position): + doc = Document(DOC_URI, workspace, DOC) + return pylsp_hover( + doc._config, doc, {"line": position[0], "character": position[1]} + )["contents"]["value"] + + +def test_hover_over_nothing(workspace): + # Over blank line + assert "" == _hover_result_in_doc(workspace, (3, 0)) + + +def test_hover_on_keyword(workspace): + # Over "class" in "class A:" + res = _hover_result_in_doc(workspace, (4, 1)) + assert "Class definitions" in res + + +def test_hover_on_variables(workspace): + # Over "int_value" in "int_value = 10" + res = _hover_result_in_doc(workspace, (31, 2)) + assert "int" in res # type + + # Over "string_value" in "string_value = "foo"" + res = _hover_result_in_doc(workspace, (32, 2)) + assert "string" in res # type + + +def test_hover_on_class(workspace): + # Over "A" in "class A:" + res = _hover_result_in_doc(workspace, (4, 7)) + assert "A()" in res # signature + assert "Docstring for class A" in res # docstring + + # Over "A" in "instance_of_a = A()" + res = _hover_result_in_doc(workspace, (33, 17)) + assert "A()" in res # signature + assert "Docstring for class A" in res # docstring + + # Over "copy_of_class_a" in "copy_of_class_a = A" - needs infer + res = _hover_result_in_doc(workspace, (34, 4)) + assert "A()" in res # signature + assert "Docstring for class A" in res # docstring + + +def test_hover_on_property(workspace): + # Over "b" in "b = 42" + res = _hover_result_in_doc(workspace, (7, 5)) + assert "int" in res # type + assert "Docstring for the class property A.b" in res # docstring + + # Over "b" in "A.b" + res = _hover_result_in_doc(workspace, (35, 24)) + assert "int" in res # type + assert "Docstring for the class property A.b" in res # docstring + + +def test_hover_on_method(workspace): + # Over "foo" in "def foo(self):" + res = _hover_result_in_doc(workspace, (10, 10)) + assert "foo(self)" in res # signature + assert "Docstring for A.foo" in res # docstring + + +def test_hover_multiple_definitions(workspace): + # Over "int_or_string_value" + res = _hover_result_in_doc(workspace, (36, 5)) + assert "```python\nUnion[int, str]\n```" == res.strip() # only type + + # Over "overload_function" + res = _hover_result_in_doc(workspace, (37, 5)) + assert ( + "overload_function(s: int) -> int\noverload_function(s: str) -> str" in res + ) # signature + assert "Docstring of overload function" in res # docstring + + def test_numpy_hover(workspace): # Over the blank line no_hov_position = {"line": 1, "character": 0} @@ -38,7 +149,9 @@ def test_numpy_hover(workspace): doc = Document(DOC_URI, workspace, NUMPY_DOC) contents = "" - assert contents in pylsp_hover(doc._config, doc, no_hov_position)["contents"] + assert ( + contents in pylsp_hover(doc._config, doc, no_hov_position)["contents"]["value"] + ) contents = "NumPy\n=====\n\nProvides\n" assert ( @@ -71,21 +184,6 @@ def test_numpy_hover(workspace): ) -def test_hover(workspace): - # Over 'main' in def main(): - hov_position = {"line": 2, "character": 6} - # Over the blank second line - no_hov_position = {"line": 1, "character": 0} - - doc = Document(DOC_URI, workspace, DOC) - - contents = {"kind": "markdown", "value": "```python\nmain()\n```\n\n\nhello world"} - - assert {"contents": contents} == pylsp_hover(doc._config, doc, hov_position) - - assert {"contents": ""} == pylsp_hover(doc._config, doc, no_hov_position) - - def test_document_path_hover(workspace_other_root_path, tmpdir): # Create a dummy module out of the workspace's root_path and try to get # a definition on it in another file placed next to it. From 5b5abf30176d09217435b0a6b3caa28375002fda Mon Sep 17 00:00:00 2001 From: Benjamin Wilhelm Date: Fri, 13 Oct 2023 09:22:41 +0200 Subject: [PATCH 2/3] Apply suggestions from code review (fix typos) Co-authored-by: tkrabel-db <91616041+tkrabel-db@users.noreply.github.com> --- pylsp/plugins/hover.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pylsp/plugins/hover.py b/pylsp/plugins/hover.py index ad4f2a56..9b13684d 100644 --- a/pylsp/plugins/hover.py +++ b/pylsp/plugins/hover.py @@ -16,9 +16,9 @@ def _find_docstring(definitions): # TODO(Review) # We could also concatenate all docstrings we find in the definitions - # I am agains this because + # I am against this because # - If just one definition has a docstring, it gives a false impression of the hover element - # - If multiple definitions have a docstring, the user will probably not relize + # - If multiple definitions have a docstring, the user will probably not realize # that he can scroll to see the other options return "" From 738ee2f74797fb7f624e02c283692d36aec95f88 Mon Sep 17 00:00:00 2001 From: Benjamin Wilhelm Date: Wed, 7 Feb 2024 12:36:14 +0100 Subject: [PATCH 3/3] Revise signatures and types on hover - Only show signatures for function and class types - Also show types if there are definitons without signatures next to definitions with signatures --- pylsp/plugins/hover.py | 64 +++++++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/pylsp/plugins/hover.py b/pylsp/plugins/hover.py index 9b13684d..e4b2483d 100644 --- a/pylsp/plugins/hover.py +++ b/pylsp/plugins/hover.py @@ -1,6 +1,7 @@ # Copyright 2017-2020 Palantir Technologies, Inc. # Copyright 2021- Python Language Server Contributors. +import itertools import logging from pylsp import _utils, hookimpl @@ -42,38 +43,49 @@ def _find_docstring(definitions): return types[0].docstring(raw=True) -def _find_signatures(definitions, word): - # Get the signatures of all definitions - signatures = [ - signature.to_string() - for definition in definitions - for signature in definition.get_signatures() - if signature.type not in ["module"] - ] - - if len(signatures) != 0: +def _find_signatures_and_types(definitions): + def _line_number(definition): + """Helper for sorting definitions by line number (which might be None).""" + return definition.line if definition.line is not None else 0 + + def _get_signatures(definition): + """Get the signatures of functions and classes.""" + return [ + signature.to_string() + for signature in definition.get_signatures() + if signature.type in ["class", "function"] + ] + + definitions = sorted(definitions, key=_line_number) + signatures_per_def = [_get_signatures(d) for d in definitions] + types_per_def = [d.infer() for d in definitions] + + # a flat list with all signatures + signatures = list(itertools.chain(*signatures_per_def)) + + # We want to show the type if there is at least one type that does not + # correspond to a signature + if any( + len(s) == 0 and len(t) > 0 for s, t in zip(signatures_per_def, types_per_def) + ): + # Get all types (also the ones that correspond to a signature) + types = set(itertools.chain(*types_per_def)) + type_names = [t.name for t in sorted(types, key=_line_number)] + + if len(type_names) == 1: + return [*signatures, type_names[0]] + elif len(type_names) > 1: + return [*signatures, f"Union[{', '.join(type_names)}]"] + + else: + # The type does not add any information because it is already in the signatures return signatures - # If we did not find a signature, infer the possible types of all definitions - types = [ - t.name - for d in sorted(definitions, key=lambda d: d.line) - for t in sorted(d.infer(), key=lambda t: t.line) - ] - if len(types) == 1: - return [types[0]] - elif len(types) > 1: - return [f"Union[{', '.join(types)}]"] - @hookimpl def pylsp_hover(config, document, position): code_position = _utils.position_to_jedi_linecolumn(document, position) - - # TODO(Review) - # We could also use Script.help here. It would not resolve keywords definitions = document.jedi_script(use_document_path=True).help(**code_position) - word = document.word_at_position(position) hover_capabilities = config.capabilities.get("textDocument", {}).get("hover", {}) supported_markup_kinds = hover_capabilities.get("contentFormat", ["markdown"]) @@ -83,6 +95,6 @@ def pylsp_hover(config, document, position): "contents": _utils.format_docstring( _find_docstring(definitions), preferred_markup_kind, - signatures=_find_signatures(definitions, word), + signatures=_find_signatures_and_types(definitions), ) }