Introduce `skip_constants` flag to `dsl` #1877

KaanOzkan · 2024-04-19T20:50:01Z

Motivation

There might be need for an escape hatch that will prevent tapioca from generation DSL RBIs for specific constants.

Implementation

Remove specified constants from the list of processable constants coming from DSL compilers

Tests

Included

egiurleo

This implementation makes sense to me! In terms of naming, how do you feel about exclude_constants? I can imagine someone seeing one of the existing options named "exclude" and then searching for that, which would help them find this one.

sambostock · 2024-04-19T21:23:57Z

With regards to naming, I would go so far as to say that we should deprecate exclude (and similar options) in both dsl and gem (in a separate PR) in favour of more explicit exclude_compilers and exclude_gems options, respectively. IMO it's non-obvious that exclude in dsl means to exclude compilers rather than files or constants.

KaanOzkan · 2024-04-19T21:26:34Z

I opted to not use exclude_* because it's closely related to exclude flag. If we're having this new flag I'd also like to specify exclude option further although it'll take some time since it's a breaking change. If that's the long term plan I think exclude_constants is better.

egiurleo · 2024-04-19T21:47:54Z

I like the idea of eventually deprecating exclude and only using exclude_xyz instead. Either way we'll have to do a release a deprecation and then a breaking change to there's time to fix stuff later.

paracycle · 2024-04-22T15:06:07Z

lib/tapioca/cli.rb

@@ -149,6 +154,7 @@ def dsl(*constant_or_paths)
        exclude: options[:exclude],
        file_header: options[:file_header],
        tapioca_path: TAPIOCA_DIR,
+        skip_constants: options[:skip_constant],


This doesn't match the name of the option above, but I would suggest that we name the option as singular anyway.

paracycle · 2024-04-22T15:07:48Z

lib/tapioca/cli.rb

@@ -135,6 +135,11 @@ def todo
      type: :boolean,
      desc: "Halt upon a load error while loading the Rails application",
      default: true
+    option :skip_constants,


I suggest this becomes:

Suggested change

option :skip_constants,

option :skip_constant,

since it is primarily meant for skipping a constant but also accepts multiple constants to be specified as part of a single option.

KaanOzkan · 2024-04-24T15:35:56Z

README.md

@@ -933,6 +934,7 @@ dsl:
  list_compilers: false
  app_root: "."
  halt_upon_load_error: true
+  skip_constant: []


Decided to go with skip_constant because I believe differentiating between exclude option and this one is valuable since exclude doesn't specify what it's excluding. And to change the name of exclude option we'd have to go through a deprecation cycle and tell users to update which I believe creates unnecessary work for little benefit.

Morriar · 2024-04-25T13:53:08Z

README.md

@@ -500,6 +500,7 @@ Options:
                                                                                                   # Default: .
             [--halt-upon-load-error], [--no-halt-upon-load-error], [--skip-halt-upon-load-error]  # Halt upon a load error while loading the Rails application
                                                                                                   # Default: true
+             [--skip-constant=constant [constant ...]]                                             # Skip the given application constant(s) when generating RBIs


While the name of the option is okay, I think we should use something else than skip in the description to better explain what it is doing:

Suggested change

[--skip-constant=constant [constant ...]] # Skip the given application constant(s) when generating RBIs

[--skip-constant=constant [constant ...]] # Do not generate RBI definition for the given application constant(s)

Morriar · 2024-04-25T13:54:19Z

lib/tapioca/commands/abstract_dsl.rb

@@ -36,6 +37,7 @@ def initialize(
        exclude:,
        file_header:,
        tapioca_path:,
+        skip_constant:,


I think this could be optional with a default value?

Suggested change

skip_constant:,

skip_constant: [],

Morriar · 2024-04-25T13:55:53Z

lib/tapioca/cli.rb

@@ -149,6 +154,7 @@ def dsl(*constant_or_paths)
        exclude: options[:exclude],
        file_header: options[:file_header],
        tapioca_path: TAPIOCA_DIR,
+        skip_constant: options[:skip_constant],


Should we check that requested_constants and skip_constant do not conflict? Or maybe add a note about which one takes precedence?

Since options themselves aren't incompatible I added a detection and warning in the pipeline while looking at its contents.

Morriar · 2024-04-25T13:56:50Z

lib/tapioca/commands/abstract_dsl.rb

@@ -124,6 +127,7 @@ def create_pipeline
          error_handler: ->(error) {
            say_error(error, :bold, :red)
          },
+          skipped_constants: constantize(@skip_constant),


Would it make sense to also path ignore_missing to constantize?

I think it's useful to tell the user that they are supplying an incorrect constant name so I kept this as is.

Morriar · 2024-04-25T13:58:03Z

spec/tapioca/cli/dsl_spec.rb

+            end
+          RB
+
+          result = @project.tapioca("dsl --skip-constant Job")


Let's add a test with more than one value for --skip-constant so we do not accidentally break this in the future.

There might be need for an escape hatch that will prevent tapioca from generation DSL RBIs for specific constants.

egiurleo reviewed Apr 19, 2024

View reviewed changes

paracycle reviewed Apr 22, 2024

View reviewed changes

KaanOzkan force-pushed the ko/dsl-skip-constant branch 2 times, most recently from 46891d7 to c532150 Compare April 23, 2024 18:57

KaanOzkan added the enhancement New feature or request label Apr 24, 2024

KaanOzkan marked this pull request as ready for review April 24, 2024 15:27

KaanOzkan requested a review from a team as a code owner April 24, 2024 15:27

KaanOzkan requested review from Morriar and vinistock April 24, 2024 15:27

KaanOzkan commented Apr 24, 2024

View reviewed changes

Morriar reviewed Apr 25, 2024

View reviewed changes

Introduce skip_constants flag to dsl

b5f2176

There might be need for an escape hatch that will prevent tapioca from generation DSL RBIs for specific constants.

KaanOzkan force-pushed the ko/dsl-skip-constant branch from c532150 to b5f2176 Compare May 1, 2024 13:52

KaanOzkan requested a review from Morriar May 1, 2024 18:43

Morriar approved these changes May 1, 2024

View reviewed changes

KaanOzkan merged commit 0007e48 into main May 2, 2024
34 checks passed

KaanOzkan deleted the ko/dsl-skip-constant branch May 2, 2024 12:40

shopify-shipit bot temporarily deployed to production May 7, 2024 15:14 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `skip_constants` flag to `dsl` #1877

Introduce `skip_constants` flag to `dsl` #1877

KaanOzkan commented Apr 19, 2024 •

edited

egiurleo left a comment

sambostock commented Apr 19, 2024

KaanOzkan commented Apr 19, 2024

egiurleo commented Apr 19, 2024

paracycle Apr 22, 2024

paracycle Apr 22, 2024

KaanOzkan Apr 24, 2024 •

edited

Morriar Apr 25, 2024

Morriar Apr 25, 2024

Morriar Apr 25, 2024

KaanOzkan May 1, 2024

Morriar Apr 25, 2024

KaanOzkan May 1, 2024

Morriar Apr 25, 2024

	[--skip-constant=constant [constant ...]] # Skip the given application constant(s) when generating RBIs
	[--skip-constant=constant [constant ...]] # Do not generate RBI definition for the given application constant(s)

Introduce skip_constants flag to dsl #1877

Introduce skip_constants flag to dsl #1877

Conversation

KaanOzkan commented Apr 19, 2024 • edited

Motivation

Implementation

Tests

egiurleo left a comment

Choose a reason for hiding this comment

sambostock commented Apr 19, 2024

KaanOzkan commented Apr 19, 2024

egiurleo commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaanOzkan Apr 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Introduce `skip_constants` flag to `dsl` #1877

Introduce `skip_constants` flag to `dsl` #1877

KaanOzkan commented Apr 19, 2024 •

edited

KaanOzkan Apr 24, 2024 •

edited