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

fix(rubocop) support projects that do not use bundler #2706

Merged
merged 1 commit into from
Jul 8, 2023

Conversation

gongfarmer
Copy link
Contributor

The existing rubocop lsp configuration uses the 'bundle exec' command as a prefix to run rubocop.
This is the correct way to do it for projects using the bundler gem (including all rails projects.)

However that configuration is broken for ruby projects not using bundler. The LSP fails to start and an error message is shown on startup.

This PR makes the default configuration work for either case, by detecting whether bundler is in use and using the correct command.

@gongfarmer
Copy link
Contributor Author

Testing done:

  • implemented change locally in ~/local/share/nvim
  • Opened nvim from a project dir not using bundler (no Gemfile)
  • :LspInfo shows cmd: rubocop --lsp
  • Opened nvim from project root dir of a rails project (has Gemfile)
  • :LspInfo shows cmd: bundle exec rubocop --lsp
  • Opened nvim from a non-root dir of a rails project
  • :LspInfo shows cmd: bundle exec rubocop --lsp

In each case, :LspLog shows no problems

@glepnir glepnir merged commit 3c0915f into neovim:master Jul 8, 2023
9 checks passed
-- prepend 'bundle exec' to cmd if bundler is being used
local gemfile_path = util.path.join(root_dir, 'Gemfile')
if util.path.exists(gemfile_path) then
config.cmd = { 'bundle', 'exec', unpack(config.cmd) }
Copy link

@tradiff tradiff Jul 8, 2023

Choose a reason for hiding this comment

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

@gongfarmer @glepnir FYI, I believe this change has broken my use-case, which was working previously. I am now unable to override cmd.

I am in a rails app, so bundle is available. But the version of rubocop installed in my rails app does not have the lsp. I would like to use the globally installed version, which I was previously doing by overriding cmd:

  cmd = { "rubocop", "--lsp", },

But now this seems to not be possible. I have also tried overriding on_new_config, but my overridden function doesn't seem to replace the default one.

lspconfig.rubocop.setup({
  cmd = { "rubocop", "--lsp", },
  on_attach = on_attach,
  capabilities = capabilities,
  on_new_config = function (new_config, new_root_dir)
    -- noop
  end,
})

This shows up in the lsp log, suggesting the default on_new_config is still active.

[ERROR][2023-07-08 15:38:29] .../vim/lsp/rpc.lua:734	"rpc"	"bundle"	"stderr"	"invalid option: --lsp\nFor usage information, use --help\n"

Copy link

@tradiff tradiff Jul 8, 2023

Choose a reason for hiding this comment

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

Update: I think I've figured this out. Providing my own on_new_config implementation doesn't override the default. Instead, both are executed. So I need to actually overwrite the effects of the default on_new_config.

My working configuration:

lspconfig.rubocop.setup({
  on_attach = on_attach,
  capabilities = capabilities,
  on_new_config = function (new_config, new_root_dir)
    new_config.cmd = { "rubocop", "--lsp", }
  end,
})

tradiff/dotfiles@8c7a862

@glepnir
Copy link
Member

glepnir commented Jul 9, 2023

I am not sure about these command. @gongfarmer could you take a look at this ?

@gongfarmer
Copy link
Contributor Author

We should delete the bundle exec part altogether, and delete the on_new_config handler that I added too so the configuration does not run twice.

Rails devs who actually want to manage their rubocop version through bundler can then configure this specially.

This would match the behaviour of the nvim-lspconfig configurations for ruby_ls and solargraph .
Neither of them does bundle exec.

I am travelling today but will put up a PR for that tomorrow.

@tradiff I do not understand why your configuration was running twice, using the old config and then the new one. This seems like an nvim-lspconfig bug to me, or at least a surprising behaviour that I was unaware of. I am not knowledgeable about nvim-lspconfig.

@glepnir Is it expected that the language server would be started twice when there is an on_new_config handler?

@tradiff
Copy link

tradiff commented Jul 9, 2023

I don't think it's starting two different instances of the language server. I think both on_new_config functions execute. The default, and then the user-supplied function. And this is all before starting a language server. A quick search through the codebase shows this, which might be the responsible code (I am not familiar with this code-base, so this is just a guess).

This might not be a "bug", but maybe room for improvement from nvim-lspconfig on the principal of least surprise.

  1. Surprise 1: I expected my cmd to take priority over anything in the default, so it was a surprise that the default configuration can overwrite the user's cmd.
  2. Surprise 2: Once I figured out what was going on, I expected my on_new_config to replace the default on_new_config. My first attempt at solving this was to assign a new function which simply does nothing. Of course, this didn't help, since the default function still ran and changed my cmd.

Looking at other server defaults, using an on_new_config to overwrite cmd in a more dynamic way isn't new to this rubocop implementation. So I don't think you did anything out of the ordinary here. It's maybe just a larger conversation about nvim-lspconfig. One idea for the first surprise is that all server defaults with on_new_config could have some way of detecting that the user has supplied their own cmd, and not overwrite it.

@gongfarmer
Copy link
Contributor Author

@tradiff You're right, I find the 2 behaviours you described surprising too.
I expect that anyone trying to configure this will likely be surprised by this.

Here is the discussion around how this is handled for solargraph:
Related: #1886

@johnpitchko
Copy link

I will confess that I don't understand everything as I am no expert in neovim, LSP or ruby. But I receive this error when I run rubocop in my rails project(s):
[ERROR][2023-09-29 23:03:02] .../vim/lsp/rpc.lua:734 "rpc" "bundle" "stderr" "invalid option: --lsp\nFor usage information, use --help\n"

Testing on my console, rubocop --lsp launches rubocop in LSP mode fine, but bundle exec rubocop --lsp fails with the error message above.

I see this seems to be the same issue @tradiff was experiencing. I'll try his workaround.

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.

4 participants