Shopify / ruby-lsp

An opinionated language server for Ruby
https://shopify.github.io/ruby-lsp/
MIT License
1.35k stars 121 forks source link

Move rubocop file watching into the server #1921

Open Earlopain opened 1 month ago

Earlopain commented 1 month ago

Motivation

For #1457. Need to figure out how to clear old diagnostics after the config has been changed. Otherwise the user can see stale diagnostics.

Implementation

Automated Tests

Manual Tests

vinistock commented 1 month ago

To clear old diagnostics, you can just publish an empty array. We do this when files are closed https://github.com/Shopify/ruby-lsp/blob/913677941c4e4746a1f9d7dc554bc899a96ab243/lib/ruby_lsp/server.rb#L278

Earlopain commented 1 month ago

This isn't really working. Even though the notification is being pushed the diagnostics still remain. In fact, if I remove the call to textDocument/publishDiagnostics diagnostics still disappear when closing the file, so it looks like this isn't really doing anything.

My theory is that since diagnostics are pulled, the client isn't really interested in what the server has to say? Not coming up with much else.

vinistock commented 1 month ago

I wonder if this isn't a bug in the language server node package. There's no mention in the spec that pull diagnostics can't be cleared by a publish notification. And the documentation for publish explicitly mentions that pushing an empty array will clear previously existing diagnostics. It might be worth asking in https://github.com/microsoft/vscode-languageserver-node.

Another option, which might work (hopefully), is to collect diagnostics after re-creating the RuboCop instance and try to push the violations using the new configuration. Although, we may find ourselves in a similar situation if no diagnostics are collected after the configuration change.

Earlopain commented 1 month ago

I'll try to check that out sometime. Gonna dig around a bit beforehand. There are a few references to publishDiagnostics, I might find something