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.

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

Return empty diagnostics result if there are no syntax errors and rubocop isn't installed #1290

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

catlee
Copy link
Contributor

@catlee catlee commented Jan 9, 2024

Motivation

In neovim/neovim#26640 we discovered that ruby-lsp sometimes sends back a nil response to textDocument/diagnostic.

I tracked this down to when we have a syntactically valid file, and we don't have rubocop available, then the diagonstic code returns nil.

According to the LSP spec, we should return an empty array in this case.

Implementation

Return an empty array if we don't have Support::RuboCopDiagnosticsRunner

Automated Tests

I added some tests here, but not sure if the way I'm manipulating the Support module is good.
I also noticed that the number of diagnostics we get back from the tests varies; sometimes we get back just rubocop diagnostics, and sometimes we get back rubocop + sorbet diagnostics.

@catlee catlee self-assigned this Jan 9, 2024
@catlee catlee marked this pull request as ready for review January 9, 2024 16:53
@catlee catlee requested a review from a team as a code owner January 9, 2024 16:53
@vinistock
Copy link
Member

Looks good, but I think you need a rebase.

@catlee catlee force-pushed the catlee/empty_diagnostics branch from 407c442 to 705361b Compare January 9, 2024 19:05
@catlee catlee force-pushed the catlee/empty_diagnostics branch from 705361b to 261a463 Compare January 9, 2024 21:11
test/requests/diagnostics_test.rb Outdated Show resolved Hide resolved
test/requests/diagnostics_test.rb Outdated Show resolved Hide resolved
test/requests/diagnostics_test.rb Outdated Show resolved Hide resolved
@egiurleo egiurleo removed their request for review January 9, 2024 22:33
@st0012 st0012 added the bugfix This PR will fix an existing bug label Jan 10, 2024
@st0012 st0012 merged commit c37838b into main Jan 10, 2024
30 of 36 checks passed
@st0012 st0012 deleted the catlee/empty_diagnostics branch January 10, 2024 15:46
@olimorris
Copy link

Amazing work @catlee!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants