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

Use default gemfile path instead of Rails root when listing engines #1018

Merged
merged 1 commit into from
Jun 28, 2022

Conversation

vinistock
Copy link
Member

Motivation

For projects that aren't Rails, such as Tapioca itself, Rails.root is nil, which then breaks when trying to invoke Rails.root.to_path.

Implementation

Add a check for verifying that Rails.root is set before continuing down to list engines. This enables Tapioca to generate RBIs for itself again.

Tests

I couldn't figure out a way of forcing Rails.root to return nil. Let me know if you have ideas.

@vinistock vinistock requested a review from a team June 28, 2022 14:56
@vinistock vinistock self-assigned this Jun 28, 2022
Copy link
Member

@paracycle paracycle left a 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 this is the correct way to go about this, since the Rails.root is not even really needed except for filtering app engines.

I'd rather that we do the same thing we do for filtering out app gems, which is to use the folder of the Gemfile instead of something like Rails.root. Ref:

def dir
File.expand_path(gemfile.path + "/..")
end

RyanBrushett
RyanBrushett previously approved these changes Jun 28, 2022
@RyanBrushett RyanBrushett dismissed their stale review June 28, 2022 15:03

Didn't see Ufuk's comment when submitted

@paracycle
Copy link
Member

paracycle commented Jun 28, 2022

Basically I'd rather we use Bundler.default_gemfile.parent.expand_path instead of Rails.root.to_path when doing the filtering.

@vinistock vinistock force-pushed the vs/fix_engine_loading_for_non_rails branch from 3538ed3 to c022ec9 Compare June 28, 2022 15:53
@vinistock vinistock changed the title Skip trying to load engines if Rails.root is not defined Use default gemfile path instead of Rails root when listing engines Jun 28, 2022
@vinistock vinistock requested a review from paracycle June 28, 2022 15:53
@vinistock vinistock merged commit d7de2f8 into main Jun 28, 2022
@vinistock vinistock deleted the vs/fix_engine_loading_for_non_rails branch June 28, 2022 17:22
vinistock added a commit that referenced this pull request Jun 28, 2022
…ails

Use default gemfile path instead of Rails root when listing engines
@paracycle paracycle added the backported Backported to stable branch label Jul 1, 2022
@shopify-shipit shopify-shipit bot temporarily deployed to production July 7, 2022 17:53 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to 0-9-stable August 19, 2022 20:37 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Backported to stable branch bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants