Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Migrate .rubocop.yml watching to the server #1457

Closed
vinistock opened this issue Mar 5, 2024 · 1 comment · Fixed by #1972 or #2982
Closed

Migrate .rubocop.yml watching to the server #1457

vinistock opened this issue Mar 5, 2024 · 1 comment · Fixed by #1972 or #2982
Labels
enhancement New feature or request help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale

Comments

@vinistock
Copy link
Member

We currently watch for changes in .rubocop.yml from the extension and simply restart the server in case the configuration was modified.

We should migrate the file watching so that it's initiated from the server, making it available in other editors too. Furthermore, we don't need to restart the server, but rather just re-initialize the RuboCop runner object to use the new configs.

Implementation

  1. We need to send another request for file watching like this one, but instead requesting to watch the .rubocop.yml file.
  2. When we receive the notification that a file has been modified, we need to add a specific check for .rubocop.yml here
  3. We need to somehow update the configurations used by the RuboCop runner, which are initialized here. Alternatively, if there's no way to reconfigure the same runner instance, we might be able to re-initialize the singleton, which will do the right thing
@vinistock vinistock added enhancement New feature or request help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale labels Mar 5, 2024
Earlopain added a commit to Earlopain/ruby-lsp that referenced this issue Apr 9, 2024
@Earlopain
Remove Singleton from formatters
bdb3d1d 
This will make it trivial to reload the rubocop config for Shopify#1457 without doing an
awkward `Singleton.__init__`.
With `GlobalState` being a thing now, I don't see the need for a singleton
Earlopain added a commit to Earlopain/ruby-lsp that referenced this issue Apr 9, 2024
@Earlopain
Remove Singleton from formatters
6f538d0 
This will make it trivial to reload the rubocop config for Shopify#1457 without doing an
awkward `Singleton.__init__`.
With `GlobalState` being a thing now, I don't see the need for a singleton
@Earlopain Earlopain mentioned this issue Apr 9, 2024
Merged
vinistock pushed a commit that referenced this issue Apr 10, 2024
@Earlopain
Remove Singleton from formatters (#1904)
3850b82 
This will make it trivial to reload the rubocop config for #1457 without doing an
awkward `Singleton.__init__`.
With `GlobalState` being a thing now, I don't see the need for a singleton
@Earlopain Earlopain mentioned this issue Apr 12, 2024
Closed
@Earlopain Earlopain mentioned this issue Apr 25, 2024
Merged
@Earlopain
Copy link
Contributor

@vinistock I notice that this was accidentally closed by the attached PR

@andyw8 andyw8 reopened this May 4, 2024
@Sean0628 Sean0628 mentioned this issue Aug 29, 2024
Closed
@vinistock vinistock mentioned this issue Dec 13, 2024
Merged
vinistock added a commit that referenced this issue Jan 7, 2025
@vinistock
Move RuboCop file watching to the server (#2982)
516cff3 
### Motivation

Closes #1457

This was originally attempted in #1921 and #2510. This PR moves watching `.rubocop.yml` to the server, so that we can avoid a full server restart and instead just create a new instance of the RuboCop runner.

This PR still suffers from the same issue reported in both previous attempts: trying to clear existing diagnostics after changing `.rubocop.yml` simply does not work. I believe this must be some sort of bug in the client or some assumption we're not satisfying. Clearing diagnostics works normally when closing files on `textDocument/didClose` notifications, so I see no reason why it wouldn't work while processing a did change watched files notification.

Despite the bug, since we're trying to improve stability of the LSP, I think it's still worth moving forward. Launching the server is still the most critical part in our entire life cycle and reducing the amount of times we restart helps prevent unnecessary pain for developers. Additionally, simply editing the file with the stale diagnostics already makes them go away immediately, so the bug is not that terrible.

I created an issue to discuss the bug with the maintainers of the protocol microsoft/vscode-languageserver-node#1594.

### Implementation

Stopped restarting the server on `.rubocop.yml` and `.rubocop` modifications. Instead, we watch the files in the server and simply re-create the instance of the runner.

Note that this has the added benefit of working on any editor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale
Projects
None yet
Milestone
No milestone
3 participants
@andyw8 @Earlopain @vinistock