Add rubocop integration

[joe-re]

I implement rubocop integraion.

Need Conversation:

• Global rubocop command is used in this PR, but I think, we should use local command when there is managed by Gemfile. If you agree with it, I'll try it later. :)
• It may be probably better to be optional configration. How do you think it? And, if you can think of a good option name, please tell me.

[mtsmfm]

Wow, I'm also thinking rubocop integration!
Great ❤️

Global rubocop command is used in this PR, but I think, we should use local command when there is managed by Gemfile. If you agree with it, I'll try it later. :)

Hmm, I think it's good to be invoked in the same process.
In other words, I'd like to load and invoke rubocop directly instead of using open3.

It may be probably better to be optional configuration. How do you think it? And, if you can think of a good option name, please tell me.

You meant "how can we enable/disable rubocop?", right?
IMO, it'll be enabled if rubocop can be required.

[joe-re]

Hmm, I think it's good to be invoked in the same process.
In other words, I'd like to load and invoke rubocop directly instead of using open3.

I agree with your thought, but I guess it's difficult...

Bundler replaces dependencies path.
A process which it was started by bundle exec can't require a global gem.

ex)

• test.rb

puts Gem::Specification._all.each(&:gem_dir)
puts require "rubocop"

In case of no bundle exec, it can require global gems.

$ ruby test.rb
# ...
#<Gem::Specification name=rubocop version=0.50.0>
#<Gem::Specification name=rubocop version=0.48.0>
#<Gem::Specification name=ruby-progressbar version=1.9.0>
#<Gem::Specification name=ruby-progressbar version=1.8.1>
#<Gem::Specification name=terminal-notifier version=1.6.3>
#<Gem::Specification name=thor version=0.20.0>
#<Gem::Specification name=unicode-display_width version=1.3.0>
#<Gem::Specification name=unicode-display_width version=1.1.3>
true

In case of bundle exec, it can't.

$ bundle exec ruby test.rb
#<Bundler::StubSpecification name=bundler version=1.14.6 platform=ruby>
test.rb:2:in `require': cannot load such file -- rubocop (LoadError)
        from test.rb:2:in `<main>'

So, we have to add rubocop and its dependencies to $LOAD_PATH to use require.
But I think it's not good.
First, dependencies gets been included in language_server, it's a huge of side effect.
Second, basically docker container can't lookup a host file. So, it can't lookup directory.

Simple solution is to add rubocop to language_server's deps. But in that case, it can't use local managed rubocop...

Do you have any good idea?

[mtsmfm]

Sorry, I meant local rubocop.
I don't recommend to install rubocop in global and wouldn't like to consider global gem for now.

I assume that the common use case is all things in one Gemfile:

gem 'rails'
...
gem 'rubocop'
gem 'language_server'

[cameronbroe]

I would think the language server would not be installed on a per project basis, as it usually has little to do with an actual project. It makes more sense for it to be installed globally as it is being referenced by a text editor, which may or may not set its current working directory as the currently opened project's directory. As such, it may make sense for it to include Rubocop as a dependency inside language_server and use a global Rubocop executable, but use the local project's Rubocop configuration. Otherwise, there are going to be multiple language_server installations on a user's machine and it becomes difficult for a text editor to keep track of where it needs to be loading the language server's executable unless we have a Ruby LSP specific extension for every editor. Letting it be global allows generic extension's like Sublime's LSP to just use it without having to constantly search for it.

[joe-re]

I agree with @cameronbroe.
I think, using LSP or not should be left to each developer.
It may become difficult to use if we compel user to add LSP deps to each project Gemfile.
I worry about to lost user, because of it.
@mtsmfm
What do you think about it?

[mtsmfm]

tl;dr
I think calling require 'rubocop' and using Rubocop::Runner or doing as same as binstub is the best way.
And I wouldn't like to include rubocop for now.
It will be disabled if we can't load rubocop.

---

I think the language server is installed as same as other tools, such like rubocop, guard, etc.
As far as I know, generally they're installed locally.

https://github.com/tootsuite/mastodon/blob/8392ddbf87f5522c445573c50e4f21d690172bc0/Gemfile#L105

And if other co-workers or contributors disagree with adding it to Gemfile, you can create local Gemfile.
https://github.com/rails/rails/blob/01ae39660243bc5f0a986e20f9c9bff312b1b5f8/.gitignore#L4

And Ruby version is different between projects.
I think syntax checking is done by the Ruby used in the project.

So IMO, it should be installed locally and how to boot the language server will depend on each project.
How to configure booting should be considered by plugins, not language server's task.

However, I wouldn't like to prevent installing this server globally.
I'd like to leave handling version to Ruby itself.
If we can find rubocop by require, let's use it.

[joe-re]

@mtsmfm ok, I implemented to try require and run rubocop on LSP process.
Could you review it?

[mtsmfm]

Do we still need this line?
I guess it's for loading rubocop binary and it isn't needed anymore

[mtsmfm]

Could you remove this line?

[mtsmfm]

file_store.path_from_remote_uri(uri) will help us

[mtsmfm]

Ah, I think we can pass the content via stdin.

https://github.com/bbatsov/rubocop/blob/834262bf83f00462333245547727e102736daccd/spec/rubocop/cli/cli_options_spec.rb#L1117

[mtsmfm]

👍 🙏