Skip to content

fix(lsp): Don't show clients with empty content in hover #33692

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

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

Conversation

joshbode
Copy link

@joshbode joshbode commented Apr 28, 2025

Problem:
LSP clients that produced no content are being shown in the hover summary (e.g. the client name is shown with no results).

Example of the problem - Typescript with multiple clients (vtsls and graphql) with empty section for graphql:

image

Another example where both servers have empty results:

image

Solution:
This PR hides clients with no content in the hover summary.

@github-actions github-actions bot added the lsp label Apr 28, 2025
@joshbode joshbode force-pushed the joshbode/remove-empty-clients-from-hover branch from 9943e58 to 2f47401 Compare April 28, 2025 11:13
@github-actions github-actions bot requested a review from MariaSolOs April 28, 2025 11:13
@joshbode joshbode force-pushed the joshbode/remove-empty-clients-from-hover branch 3 times, most recently from 8d06d0e to abb3ee5 Compare April 28, 2025 11:32
@justinmk justinmk added the needs:response waiting for reply from the author label Apr 28, 2025
@joshbode joshbode force-pushed the joshbode/remove-empty-clients-from-hover branch from abb3ee5 to e5cfea9 Compare April 28, 2025 12:25
@joshbode joshbode changed the title feat(lsp): Don't show clients with empty diagnostic content in hover fix(lsp): Don't show clients with empty diagnostic content in hover Apr 28, 2025
@joshbode joshbode changed the title fix(lsp): Don't show clients with empty diagnostic content in hover fix(lsp): Don't show clients with empty content in hover Apr 28, 2025
@joshbode joshbode force-pushed the joshbode/remove-empty-clients-from-hover branch from e5cfea9 to eabf40c Compare April 28, 2025 20:49
Copy link
Member

@MariaSolOs MariaSolOs left a comment

Choose a reason for hiding this comment

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

I'm not sure how I feel about this. It feels wrong for a server to return an empty hover response when it could return null instead. I think that if a server decides to return empty content, we should display it.

results1[client_id] = result
local contents = result.contents
-- Ensure that there is some content to display
if type(contents) == 'string' or contents.kind or contents.language or contents[1] then
Copy link
Member

Choose a reason for hiding this comment

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

A comment explaining that this corresponds to checking that contents is a MarkedString | MarkedString[] | MarkupContent would be appreciated.

Copy link
Author

Choose a reason for hiding this comment

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

I've simplified the check to just make sure #contents > 0 (i.e. it is not an empty table or an empty string)

@justinmk
Copy link
Member

feels wrong for a server to return an empty hover response when it could return null instead. I think that if a server decides to return empty content, we should display it.

good point. is this actually common, or is it a server quirk ?

@joshbode joshbode force-pushed the joshbode/remove-empty-clients-from-hover branch from eabf40c to 313e543 Compare April 29, 2025 22:20
@joshbode
Copy link
Author

feels wrong for a server to return an empty hover response when it could return null instead. I think that if a server decides to return empty content, we should display it.

good point. is this actually common, or is it a server quirk ?

I'm not sure if it's common, but anecdotally, both vtsls and graphql seem to always produce a content array (which is often empty, especially for graphql since it only applies to embedded GraphQl queries).

@github-actions github-actions bot removed the needs:response waiting for reply from the author label Apr 29, 2025
*Problem*:

LSP clients that produced no diagnostic results are being shown in the
diagnostics hover (e.g. the client name is shown with no results).

*Solution*:

This PR hides clients with no diagnostic content in the hover summary.
@joshbode joshbode force-pushed the joshbode/remove-empty-clients-from-hover branch from 313e543 to dc897b0 Compare April 29, 2025 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants