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

docs: how to make diagnostics work with ruby-lsp #2498

Closed
wants to merge 1 commit into from

Conversation

cefigueiredo
Copy link

ruby-lsp >= 0.2.2 does not implement textDocument/publishDiagnostic specification that is supported by Neovim, implementing only the textDocument/diagnostic.
This commit presents a workaround to make ruby_ls send textDocument/diagnostic requests to the Server, and treat the response like it was for a textDocument/publishDiagnostic event.

@glepnir
Copy link
Member

glepnir commented Mar 9, 2023

I think it would be better in ruby-lsp doc or somewhere then we can use that link .

@cefigueiredo
Copy link
Author

I believe the folks on ruby-lsp might be interested in linking instructions of how to integrate to other editors besides vscode, that they already maintain.

However, I understand that is also important update the usage instructions present on nvim-lspconfig docs that are accessible from the editor with :h lspconfig-all.
I identified the need for the update, because the current instructions would not completely enable the diagnostics feature for a more recent ruby-lsp version.

More specifically since ruby-lsp v0.2.2 that according to Shopify/ruby-lsp#242 dropped support to textDocument/publishDiagnostics natively used by neovim, in favor of only maintain the textDocument/diagnostic from LSP v3.17 specification that requires the client to decide when to pull for diagnostics.

@glepnir
Copy link
Member

glepnir commented Mar 9, 2023

I think the point is we should implement handler for textdocument/diagnostic and workspace/diagnostic in core..

@glepnir
Copy link
Member

glepnir commented Mar 9, 2023

merge this first . but please create an issue in core for create handler for textdocument/diagnostic

lua/lspconfig/server_configurations/ruby_ls.lua Outdated Show resolved Hide resolved
lua/lspconfig/server_configurations/ruby_ls.lua Outdated Show resolved Hide resolved
lua/lspconfig/server_configurations/ruby_ls.lua Outdated Show resolved Hide resolved
@cefigueiredo cefigueiredo force-pushed the ruby_ls-diagnostics branch 2 times, most recently from 44c9c24 to 56e41c6 Compare March 10, 2023 08:20
However, ruby-lsp since the version `0.2.2` does not support this
specification for diagnostics.
Instead, it implements the `textDocument/pullDiagnostic` specification,
that expects the client to send `textDocument/diagnostic` requests to the server
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a huge amount of documentation (for one lang server....), do we need a feature request in Nvim core? Why does it only work with old versions of ruby-lsp ? Is Nvim LSP missing a standard LSP feature?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Nvim LSP missing a standard LSP feature?

this is feature in lsp 3.17 .. as far as i know we missing some lsp feature like ontypeformat workspace/diagnostic workspace/diagnostic/refresh textDocument/diagnostic etc ..

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justinmk You are right about nvim core and lspconfig missing some features from LSP, but even if the feature get requested, it will have a long roadmap to wait for until lspconfig be able to work again as the current documentation is describing.
I forgot to make the feature request as recommended by @glepnir previously, but I think that in the meantime, we would like to complement the documentation for this lang server, just because if someone follow the instructions on it will not get lspconfig working as described.

That is because the current versions for this lang server implements only Pull Diagnostic from LSP v3.17 and dropped support to textDocument/publishDiagnostic, which is the only currently supported by Nvim.

There are alternatives to these extensive instructions, but any alternative would have a drawback of any sort:

  • Make the instructions limit to use a version of ruby-lsp <= 0.2.1
    • That is the highest version the current version of nvim/lspconfig is compatible with
    • That would limit users capacity to benefit of eventual security patches and other features that nvim might be compatible already but are only present on more recent versions for ruby-lsp
  • Nvim core and lspconfig implement textDocument/diagnostic from Lsp v3.17
    • Besides it being the best scenario, some would need to wait for the roadmap for both core and lspconfig to be released to have diagnostic working again as described in the current instructions.
    • This is the only lang server that I'm aware that does not support textDocument/publishDiagnostic anymore. I don't know if it's something that will be dropped by LSP and would increase the priority, but so far the priority might be pretty low if ever implemented.

Copy link

@smarquez1 smarquez1 Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename the server to ruby_lsp instead of ruby_ls?

Copy link
Member

@dundargoc dundargoc Apr 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes less sense IMO. We are using the language server for ruby, the protocol is just a boring specification.

Edit: wait a minute, the language server is called ruby-lsp? bruh... OK, maybe this kinda makes sense then.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dundargoc yes, the name of this language server is ruby-lsp and it's not the only language server for ruby supported by nvm-lspconfig. lspconfig also supports solargraph, the other language server for ruby.

@smarquez1
Copy link

Hey @cefigueiredo thank you for the instructions. I created a core ticket yesterday to add the textDocument/diagnostic feature to neovim using your explanation on the issue. Let me know if you want to add anything!

@technicalpickles
Copy link

Would it be reasonable to just added the recommended workaround to lua/lspconfig/server_configurations/ruby_ls.lua rather than documenting the workaround? That way, users get a better out of box experience until there is support in neovim itself.

I can think of a few ways to detect the older versions, but they are kinda dirty. ruby-lsp doesn't have a --version flag unfortunately. It would be in someone's Gemfile.lock if they have it for their project, but that isn't always how it's installed.

Maybe an option to work for new vs old ruby-lsp?

@cefigueiredo
Copy link
Author

Would it be reasonable to just added the recommended workaround to lua/lspconfig/server_configurations/ruby_ls.lua rather than documenting the workaround? That way, users get a better out of box experience until there is support in neovim itself.

I don't believe it would be reasonable. Because it would either be too invasive for the users or just not work on most cases.

This lua/lspconfig/server_configurations/ruby_ls.lua only sets the default configuration object that lsp config merges with the settings the user send when declaring the lsp client, on

nvim_lsp["ruby_ls"].setup {
  on_attach = function(_, bufnr)
    -- user custom keymaps
  end
}

nvim-lspconfig does not setup any key mappings in the user editor.
It's a premise on lspconfig to the neovim user pass to nvim_lsp["CLIENT"].setup a table structure with a on_attach function that runs when the lsp client is bound to a buffer, and it's where the user declares its own keymap bindings.

The table provided by the user is merges onto the default settings of the lsp client, replacing the table keys that match.

So, if the default settings for ruby-lsp declares its own on_attach , it would be replaced by the user's on_attach, and the user would still be unaware of how to do the workaround without the instructions in the doc for the ruby-lsp client.

To make the server client default settings declare a function to be run when the client is loaded in the buffer, but not affecting the on_attach, it would demand a complete new feature that is not the proposal for this PR, even because currently only ruby-lsp demands such workaround.

The ideal, is neovim-core support the PullDiagnostic feature from LSP specification out of the box, when that happens, the workaround would not be needed anymore. But this will take a while to enter the core roadmap.

Until then, I believe that the instructions in the doc would be the more reasonable option.

Updates the docs instructing how to make `ruby_ls` diagnostics
work with recent versions of `ruby-lsp`.

Co-authored-by: Sergio Marquez <marquez.sergio.d@gmail.com>
@justinmk
Copy link
Member

If there's a bug in Nvim LSP client, check Nvim 0.10 (development version) and say how to reproduce the issue. Adding tons of special-case notes and configs to nvim-lspconfig is out of scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants