-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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): support textDocument/diagnostic specification #7900
Conversation
507a24e
to
c56b8b8
Compare
c56b8b8
to
2aff024
Compare
f4d6635
to
8d64610
Compare
Few notes for easier review. I took a large piece of code from Added |
8d64610
to
1830372
Compare
Implementation of pull diagnostics introduced in LSP 3.17.
1830372
to
ac73099
Compare
I think it makes sense to delay this until the event system (#8021) lands. We really only want to pull diagnostics of all visible documents (only when the contents of any document actually changed, likely with a debounce, if Additionally, the support implemented here is still lacking some features that are needed to make pull diagnostics really work:
I think the last two points are pretty optional but the first two capabilities seem pretty essential to me. |
pub fn set_publish_diagnostic(&self, val: bool) { | ||
self.supports_publish_diagnostic | ||
.fetch_or(val, atomic::Ordering::Relaxed); | ||
} | ||
|
||
/// Whether the server supports Publish Diagnostic | ||
pub fn publish_diagnostic(&self) -> bool { | ||
self.supports_publish_diagnostic | ||
.load(atomic::Ordering::Relaxed) | ||
} | ||
|
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.
Instead of figuring out of the server has sent a publish diagnostic notification we can simply assume the server will not send them if it has the pull diagnostic capability set (the server will then look at our capabilities and make a decision based on that)
@pascalkuthe thanks for the extensive answer. I agree with you. I'm going to rework this PR after the event system is deployed. If for some reason I miss this part, feel free to ping.✌️ |
@woojiq looks like the event system was merged recently |
I will have a look. What language server do you use that you need pullDiagnostic, ruby-lsp? |
Planning to, yes. Right now I'm using lsp mode in rubocop, but it lacks some stuff |
Unfortunately, I am no longer interested in it for two reasons:
Although implementing this probably shouldn't be that difficult since almost all the building blocks for working with LSP are already in the codebase. I might come back to it in the summer, but point 1 is crucial. Sorry for the false expectations. |
Seems from version 3.0.1 |
Closes #7757
Explanation for easier review, see #7900 (comment).
Tested manually with
ruby-lsp
.