-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat(lsp): add related_information to vim.Diagnostic #28640
base: master
Are you sure you want to change the base?
Conversation
I think instead of extending the message this should go into the structure, otherwise users can't customise this. (And I'd wager there's a bunch who don't want this in the message). There are also actions where we send the diagnostic back to the server and I think for the server to be able to re-associate the payloads with their internal state the diagnostic mustn't change. |
7d44a02
to
e240056
Compare
Sounds fair - I changed the PR so that now it only adds the If this looks good, I'll add a test to |
If this does need to be in the top level structure it should follow the standard naming conventions ( |
Yeah to me this seems general enough (I could imagine something like overseer.nvim make use of this). Thanks for the hint related to naming conventions, fixed! |
e240056
to
a74ca2c
Compare
a74ca2c
to
b598c73
Compare
27a1070
to
402b07d
Compare
71690ab
to
a02d006
Compare
Thanks for the review! I extended the tests in |
6c5f2ce
to
5860c5a
Compare
runtime/lua/vim/diagnostic.lua
Outdated
--- Related message and location for a diagnostic. | ||
--- This can be used to point to code locations that cause or are related to | ||
--- a diagnostic, e.g when duplicating a symbol in a scope. | ||
--- @class vim.DiagnosticRelatedInformation |
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.
How about having a namescope, like vim.diagnostic.RelatedInformation
?
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.
Good point, done
5860c5a
to
d3ae14c
Compare
For now this just adds the field to the toplevel Diagnostic structure and fills it with the corresponding struct from LSP. We can decide in a follow-up if and how we should display these to the user.
d3ae14c
to
3c44e73
Compare
• Add `vim.Diagnostic.related_information` field and fill it with | ||
`lsp.DiagnosticRelatedInformation[]`. Deprecate |
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.
• Add `vim.Diagnostic.related_information` field and fill it with | |
`lsp.DiagnosticRelatedInformation[]`. Deprecate | |
• Add `vim.Diagnostic.related_information` field and fill it with | |
`vim.diagnostic.RelatedInformation[]`. Deprecate |
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.
I intentionally wrote lsp.DiagnosticRelatedInformation
here though, to highlight the new logic I added in lsp/diagnostic.lua
Basically a simplified version of #19649 (comment)
Looking to get some initial feedback here. Instead of just appending to the message, we might instead want to store the related info in the
vim.Diagnostic
struct, and then instead extend the logic invim.diagnostic.open_float()
...?This PR:
vscode:
EDIT: PR now only adds the related_information field to the toplevel of vim.Diagnostic (see discussion below). We could then decide in a follow-up PR if and how we should display this to the user.