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

Allow multiple hover results #387

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Richardk2n
Copy link
Member

The language server already handles lists as contents, so this simple change allows for multiple results (e.g., from plugins) to be presented to the user.

This is interesting for use cases like python-lsp/pylsp-mypy#63

@Richardk2n
Copy link
Member Author

Maybe this should be extended to also allow lists from plugins. What do you think of this PR in general @ccordoba12 ?

Copy link
Contributor

@krassowski krassowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an advantage to allowing returning list over the hook returining a dict with list on contents (as is already possible)?

    1. the proposed change seems to possibly add maintenance overhead (e.g. what if someone wants to return multiple contents but also adds range?)
    1. The specs indeed allows a list of MarkedStrings but those are deprecated, so I am not convinced if we should add extra behaviour depending on it (unless we can update spec to allow for MarkupContent[] too)

For reference:

/**
 * The result of a hover request.
 */
export interface Hover {
	/**
	 * The hover's content
	 */
	contents: MarkedString | MarkedString[] | MarkupContent;

	/**
	 * An optional range is a range inside a text document
	 * that is used to visualize a hover, e.g. by changing the background color.
	 */
	range?: Range;
}

@Richardk2n
Copy link
Member Author

Richardk2n commented Jul 3, 2023

Currently, (as I understand it) the server accepts one hover response

@hookspec(firstresult=True)

As one of the included plugins gives an empty response by default
return {'contents': ''}

it is not possible for 3rd party plugins to use the hover functionality.

I would like to change that.

It is possible for one plugin to return a list of contents. What my PR does (with significant room for improvement) is merging the output of multiple plugins into one such list of contents.

I was not aware of the details of the spec (did not know about the range). Can you please point me to the relevant document?
ii) If you can do that easily, that would possibly be a sensible change, but I fear your point i) still stands. Associating one range with multiple results is questionable. They would often be identical and one could argue that it makes sense to merge them, but neither is a particularly correct solution. Logically multiple such results would need to be emitted by a single request, which is probably not feasible.

So to conclude, I still would like to see this functionality extended to allow results from multiple plugins (at the same time), but I do not know how to do this properly. If you can point me in the direction of a solution, I would gladly implement it. Otherwise, feel free to close this.

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.

2 participants