-
Notifications
You must be signed in to change notification settings - Fork 137
clear diagnostics on close document #1135
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this!
It seems that CI is blocking merge. Can you take a look at it? thanks!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## main #1135 +/- ## ========================================== - Coverage 81.25% 81.19% -0.06% ========================================== Files 29 29 Lines 1408 1409 +1 Branches 334 334 ========================================== Hits 1144 1144 - Misses 217 218 +1 Partials 47 47 ☔ View full report in Codecov by Sentry. |
curious to ask: how such 'clear' helped?
just send a empty [] to client? what's the different if just do nothing when closing?
or was it helpless but adding a bit burden?
Quote from lsp specification
Diagnostics are "owned" by the server so it is the server’s responsibility to clear them if necessary. The following rule is used for VS Code servers that generate diagnostics:
- if a language is single file only (for example HTML) then diagnostics are cleared by the server when the file is closed. Please note that open / close events don’t necessarily reflect what the user sees in the user interface. These events are ownership events. So with the current version of the specification it is possible that problems are not cleared although the file is not visible in the user interface since the client has not closed the file yet.
- if a language has a project system (for example C#) diagnostics are not cleared when a file closes. When a project is opened all diagnostics for all files are recomputed (or read from a cache).
clear here usually means send empty diagnostics (i.e. []) to client, so that client's UI won't show any diagnostics on this file.
AFAIK, bash-lsp currently does not have very much project system concept, so it's resonable to clear diagnostics.
If server does not clear diagnostics, the diagnostics will live forever in client's UI, even after user has deleted the file, util user restart the extension host. This is kind of annoying, in my opinon.
Here's a video illustration:
Screen.Recording.2024年05月03日.at.23.29.52.mov
Does sending [] to client not clear diagnostics on your side? That works well on my side.
Here's a illustration: (I'm using vscode version 1.89.0 with bash-lsp on main 81125b8)
Screen.Recording.2024年05月04日.at.00.45.41.mov
P.S. this clear diagnostics behavior might be different among different editors, since they implement language client differently. If there are any cross-client compatible methods to clear diagnostics, i'd like to hear.
i am seeing your video now, i think it's probably not a problem,
i am not a vsc user, but i guess it's its same behavior for others langs.
// sending a fake [] to client i supposed it is not a clear, or if you really like to hide it then it maybe a just client option.
@skovhus so did you plan to keep this, or actually it should be a vscode option to hide it (if there was one) ?
I haven’t seen this causing any harm. Have you?
i felt it sent fake [] to client would make it add a bit burden to ACK it,
// or maybe a bit mess thoughg it depends on how client impl it.
and such actually didnot clear anything, but just hide it.
but yea, the impact seems was tiny, so if you're ok, then let it be.
Currently, bash-language-server does not clear diagnostics when user close an document. This causes bad user experience.
This PR fixes this problem, it makes sure that server clear diagnostics when document was closed