-
Notifications
You must be signed in to change notification settings - Fork 25
Use bundle-locked gem versions in child contexts #819
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
Conversation
d4ea64d to
27c3fd1
Compare
27c3fd1 to
b3fa636
Compare
b3fa636 to
fcfce11
Compare
| # Generate a gem requirement for the given gem name, using that gem's version in the "real" current bundle. | ||
| # | ||
| # This ensures that any child Spoom::Contexts use predictable gem versions, | ||
| # without having to manually specify them and bump them to stay in sync with Spoom's real Gemfile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this explanation be clear to someone who didn't already understand what's going on here?
b0b5104 to
519953f
Compare
| source "https://rubygems.org" | ||
| gem "sorbet" | ||
| #{Spoom::BundlerHelper.gem_requirement_from_real_bundle("sorbet")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we really care which version of Sorbet gets installed in these tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so either, but I'd rather not have to worry about it.
| gem "sorbet" | ||
| gem "sorbet-runtime" | ||
| #{Spoom::BundlerHelper.gem_requirement_from_real_bundle("sorbet")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I don't think we really care about which version we use
lib/spoom/bundler_helper.rb
Outdated
| if 1 < specs.count | ||
| raise <<~MSG | ||
| Found multiple versions of #{gem_name.inspect} in the current bundle: | ||
| #{specs.sort_by(&:version).map { |spec| " - #{spec.name} #{spec.version}" }.join("\n")} | ||
| MSG | ||
| end | ||
|
|
||
| unless (spec = specs.first) | ||
| raise "Did not find gem #{gem_name.inspect} in the current bundle" | ||
| end | ||
|
|
||
| %(gem "#{spec.name}", "= #{spec.version}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is such a convoluted way to express something simple:
| if 1 < specs.count | |
| raise <<~MSG | |
| Found multiple versions of #{gem_name.inspect} in the current bundle: | |
| #{specs.sort_by(&:version).map { |spec| " - #{spec.name} #{spec.version}" }.join("\n")} | |
| MSG | |
| end | |
| unless (spec = specs.first) | |
| raise "Did not find gem #{gem_name.inspect} in the current bundle" | |
| end | |
| %(gem "#{spec.name}", "= #{spec.version}") | |
| if specs.count < 1 | |
| raise "Did not find gem #{gem_name.inspect} in the current bundle" | |
| elsif specs.count > 1 | |
| raise <<~MSG | |
| Found multiple versions of #{gem_name.inspect} in the current bundle: | |
| #{specs.sort_by(&:version).map { |spec| " - #{spec.name} #{spec.version}" }.join("\n")} | |
| MSG | |
| else | |
| %(gem "#{spec.name}", "= #{spec.version}") | |
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot spec = specs.first, but then that doesn't typecheck because it's nilable, so you need a #: !nil...
And it turns out about the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So something like this?
specs = Bundler.load.gems[gem_name]
if specs.nil? || specs.empty?
raise "Did not find gem #{gem_name.inspect} in the current bundle"
elsif specs.count > 1
raise <<~MSG
Found multiple versions of #{gem_name.inspect} in the current bundle:
#{specs.sort_by(&:version).map { |spec| " - #{spec.name} #{spec.version}" }.join("\n")}
MSG
else
spec = specs.first
%(gem "#{spec.name}", "= #{spec.version}")
endThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, except it would need spec = specs.first #: !nil, which is worse. Turns out we don't need it, because Bundler.load is untyped 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would need spec = specs.first #: !nil, which is worse
Is it really now that it's a comment? Only the static checker has to do additional work, the runtime stays unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a matter of personal taste, I prefer the if (x = a.first) to check a.count > 1 and get the first in a single shot. Won't someone think of the branches?! :D
Had to also run: ```sh bundle exec spoom srb sigs export ```
RubyLSP highlights it better in this case, because it misinterprets the interpolated bits as Ruby comments (because of the leading `#`).
519953f to
7120179
Compare
Fixes CI in other projects, like https://github.com/Shopify/rbi/actions/runs/19509877786/job/55846512135?pr=534