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

YardMap#spec_for_require: Get gem with version from bundle #509

Merged
merged 1 commit into from
May 21, 2022

Conversation

alisnic
Copy link
Contributor

@alisnic alisnic commented Nov 29, 2021

This is another stab at problem described in #508. Which basically makes solargraph work better when being run outside bundle (without bundle exec)

Why run solargraph outside bundle anyway?

Legacy projects usually have gems with old gem dependencies. This means that we want to use the latest solargraph version, we can't, because one of its depedencies cannot be satisfied:

Bundler could not find compatible versions for gem "diff-lcs":
  In snapshot (Gemfile.lock):
    diff-lcs (= 1.3)

  In Gemfile:
    rspec-rails was resolved to 3.5.2, which depends on
      rspec-expectations (~> 3.5.0) was resolved to 3.5.0, which depends on
        diff-lcs (>= 1.2.0, < 2.0)

    solargraph (~> 0.44.2) was resolved to 0.44.2, which depends on
      diff-lcs (~> 1.4)

Running `bundle update` will rebuild your snapshot from scratch, using only
the gems in your Gemfile, which may resolve the conflict.

Usually upgrading deps in such projects is a huge undertaking, which often does not justify installing solargraph

What this fix does?

We already know the version of each gem from the bundle from YardMap#process_gemsets. So instead of fetching gem specification using Gem::Specification.find_by_name(name) (which, while in bundler will resolve to the only and correct version of the gem), we can fetch it using Gem::Specification.find_by_name(name, version), which resolve to correct gem version even when solargraph runs outside bundle

Impact

Before fix in a sample project with Rails annotations:

[10] pry(#<Repl>)> yard_map.yardocs.size
=> 39
[11] pry(#<Repl>)> api_map.pins.select {|p| p.path && p.path.include?("ActionController") }.map(&:path)
=> 6

After fix in a sample project

[3] pry(#<Repl>)> yard_map.yardocs.size
=> 42
[4] pry(#<Repl>)> api_map.pins.select {|p| p.path && p.path.include?("ActionController") }.map(&:path).size
=> 565

@castwide
Copy link
Owner

castwide commented Dec 5, 2021

Thanks, partner! This looks correct to me. I'm not ready to merge it yet because of other pending changes, but I wanted to make sure you knew it was on my radar and will probably be included in the next release.

@alisnic
Copy link
Contributor Author

alisnic commented Dec 5, 2021

awesome! thank you!

@castwide castwide merged commit 69d0305 into castwide:master May 21, 2022
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.

2 participants