Include non-development gemspec dependencies in DependencyDetector check

[andyw8]

Motivation

Addresses the server side of #1635. We will need another change for the extension: Shopify/vscode-ruby-lsp#814

Note

We should aim to launch this alongside the extension update, to avoid inconsistent behaviour.

Implementation

Using the same approach as we already have in SetupBunder#load_dependencies.

Automated Tests

Updated

Manual Tests

Start with the current (released version of Ruby LSP). We'll use Spoom verifiying.

In Spoom's gemspec, change minitest and minitest-reporters to be add_dependency instead of add_development_dependency. Open a test and confirm the code lens is not showing.

Now, point rubyLsp.branch to this branch. Confirm the code lenses.

[andyw8]

@vinistock what do you think of this approach? The other way might be to keep Bundler.locked_gems.dependencies but separately read from .gemspec, then aggregate them.

[paracycle]

The gemspec should have already been loaded by the gemfile, so I don't think finding it and loading it is the best way to do this.

I would instead recommend something like:

Bundler.locked_gems.sources.find { Bundler::Source::Gemspec === _1 }&.gemspec&.dependencies

[andyw8]

Yes, that seems cleaner. @vinistock, should we update the load_dependencies method to do the same?

[andyw8]

Just for illustration, with

  spec.add_dependency("minitest", "~> 5.0")
  spec.add_dependency("minitest-reporters")

we get:

irb(main):005:0> Bundler.locked_gems.dependencies.keys
=> ["bundler", "debug", "rake", "rubocop-shopify", "rubocop-sorbet", "spoom", "tapioca"]
irb(main):006:0> Bundler.load_gemspec(gemspec_path).dependencies.map(&:name)
=> ["bundler", "minitest", "minitest-reporters", "rake", "erubi", "sorbet-static-and-runtime", "syntax_tree", "thor"]

[vinistock]

Let's make sure it is really equivalent and that we get the same results. In case we do, are you talking about load_dependencies in SetupBundler?

[andyw8]

Yeah SetupBundler#load_dependencies. But on second thoughts, let's keep this PR focused on the DependencyDetector.

[vinistock]

Bundler supports more than one gemspec in the same project, so I think this needs to be something like

Bundler.locked_gems.sources.each_with_object([]) do |source, dependencies|
  next unless Bundler::Source::Gemspec === source

  dependencies.concat(source.gemspec&.dependencies.map(&:name))
end

[andyw8]

👍 Updated in 8af680d

[paracycle]

There is no need for each_with_object here:

Bundler.locked_gems.sources.flat_map do |source|
  next unless Bundler::Source::Gemspec === source

  Array(source.gemspec&.dependencies).map(&:name)
end.compact

[paracycle]

Well, even better by doing the filtering first and then doing a flat map:

Bundler.locked_gems.sources.grep(Bundler::Source::Gemspec).flat_map { |source| Array(source.gemspec&.dependencies) }.map(&:name)

[andyw8]

That's almost what I had in 8af680d, but grepping by the class name is nice, I've updated.

[vinistock]

I honestly, don't think any of those options are more readable than using each_with_object - in addition to them allocating more arrays. But whatever you all decide is fine.