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

--exclude should also exclude transitive dependencies #2096

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rzane
Copy link
Contributor

@rzane rzane commented Nov 27, 2024

Motivation

Given this gemfile:

source 'https://rubygems.org'
gem 'rspec'

The following configuration should not generate any RBI files, but it does.

gem:
  exclude:
  - rspec

This would generate RBI files for all of RSpec's transitive dependencies (e.g. rspec-core, rspec-support, rspec-mocks, rspec-expectations, diff-lcs).

Implementation

I just dropped all of the excluded gems from definition.locked_gems.dependencies.

I'm not sure if this is actually the right solution, because there's something going on with BundlerExt::AutoRequireHook.override_require_false(exclude: @excluded_gems) that I don't fully understand.

Tests

I updated the existing CLI test for the --exclude option. I've also test this against a real project, and it seems to work.

@rzane rzane requested a review from a team as a code owner November 27, 2024 16:37
@rzane
Copy link
Contributor Author

rzane commented Nov 27, 2024

I realize that a potential solution would be to add all of the transitive dependencies to the exclude list. But, this becomes impractical very quickly. For example, excluding the rails gem would require you to list out ~60 dependencies.

@vinistock
Copy link
Member

We should add a test to ensure that if two gems have the same transitive dependency, excluding one of them will not exclude the transitive dependency. For example:

foo -> actionpack
bar -> actionpack

Excluding `bar` should not exclude `actionpack` because `foo` still depends on it

I think the code is probably already handling that, but I don't know if we have a test for it.

assert(Dir[path].any?)
assert(@project.glob(path).any?)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't doing what it was supposed to be doing.

@rzane rzane force-pushed the rzane/exclude-should-ignore-transitive-dependencies branch from 21c24d8 to d9bfbc4 Compare November 27, 2024 17:16
@rzane rzane force-pushed the rzane/exclude-should-ignore-transitive-dependencies branch from d9bfbc4 to 47032ce Compare November 27, 2024 17:21
@rzane
Copy link
Contributor Author

rzane commented Nov 27, 2024

Thanks for the suggestion, @vinistock. I added a transitive dependency that is shared by an excluded gem and a gem that isn't excluded (activemodel).

@@ -77,7 +77,7 @@ def initialize(
def gems_to_generate(gem_names)
return @bundle.dependencies if gem_names.empty?

gem_names.each_with_object([]) do |gem_name, gems|
(gem_names - @exclude).each_with_object([]) do |gem_name, gems|
Copy link
Contributor Author

@rzane rzane Nov 27, 2024

Choose a reason for hiding this comment

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

This change preserves the existing behavior covered by this spec.

Without this change, invoking tapioca gem foo --exclude foo would fail with this error, which actually seems like pretty reasonable behavior to me. I'm not sure why you'd specify the name of a gem that you're excluding.

Error: Cannot find gem 'foo'

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