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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid raising when constantizing from paths #1414

Merged
merged 1 commit into from
Mar 9, 2023

Conversation

jeffcarbs
Copy link
Contributor

@jeffcarbs jeffcarbs commented Feb 24, 2023

Motivation

#968 added support for generating DSL RBI via a path like bin/tapioca dsl app/models. The way it works is using Static::SymbolLoader.symbols_from_paths to find all constants defined in those paths and then acting as if you called bin/tapioca dsl <those constants>.

When testing this out for the first time, I realized that sometimes symbols_from_paths found constants that weren't actually loaded (or loadable) and would then raise an exception. Two common examples in our codebase:

  1. A constant defined inside an eigenclass, for example
class Foo
  class << self
    BAR = nil
  end
end

In this example, Foo::BAR is a constant returned from symbols_from_paths but you can't actually reference it outside the eigenclass.

  1. Any constants (e.g. helper modules) defined in test folders. We use packwerk so my first command was something like bin/tapioca dsl packs/xyz which is a folder than contains both app and spec so it found a few constants from tests and blew up trying to load them.

Implementation

When generating DSL for specific constants like bin/tapioca dsl Some::Constant we raise if that constant can't be resolved. This makes sense since you're explicitly asking us to generate DSL for a specific constant.

We previously had the same behavior for constants derived from given paths, however this PR changes that behavior so we'll simply ignore constants that cannot be resolved. This makes sense to me since by doing bin/tapioca dsl some/path you're essentially saying "generate RBI for whatever you can in some/path".

Tests

Added a test for the eigenclass case and confirmed it failed before and passed after this change. I also tested out this branch of tapioca on Gusto's codebase and it worked as expected 馃憤

@jeffcarbs jeffcarbs requested a review from a team as a code owner February 24, 2023 03:54
@@ -804,8 +804,6 @@ class Post
OUT

assert_equal(<<~ERR, result.err)

Warning: No constants found in: path/to/nowhere.rb
Copy link
Contributor Author

@jeffcarbs jeffcarbs Feb 24, 2023

Choose a reason for hiding this comment

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

I intentionally removed this warning since we already logic to show an error for bad paths (see #968 (comment) for more details).

Copy link
Contributor

@egiurleo egiurleo left a comment

Choose a reason for hiding this comment

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

Thanks @jeffcarbs!

@egiurleo egiurleo merged commit be04e90 into Shopify:main Mar 9, 2023
@jeffcarbs jeffcarbs deleted the dsl-by-path-2 branch March 9, 2023 23:40
@shopify-shipit shopify-shipit bot temporarily deployed to production March 10, 2023 16:06 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants