Allow the default_command displayed on failure to be overridden at the command line

[kmcphillips]

Motivation

When a command like rbi --verify is run, it prints out the default_command(command) on failure to give instructions on how to regenerate RBIs and such. It is instructions to the user.

Unfortunately, in cases where the command passed in has complex arguments, it just prints the default command. This is unhelpful. An example from Shopify:

This should print the command that is on the first line.

Implementation

Allow a default_command_override to be passed in at the command line which overrides what is printed on failure, and what is printed in the headers.

We chose to pass this through basically everywhere for completeness, but this caused a ton of signatures to change. I will comment inline on the actual important changes.

Paired with @Morriar

[kmcphillips]

This is the important change.

[paracycle]

To give historical context, we used to have this feature and then we removed it since the default command also gets encoded in the RBI file headers, which was creating unnecessary noise: https://github.com/Shopify/tapioca/pull/259/files

I understand the need for this, but I also think we want to be careful about how we want to implement this so that we balance the different needs carefully.

[paracycle]

As I said in my comment above, I think this will end up changing what is printed inside the generated file headers as well, and I am not sure if we want that again.

If we do want it, though, we should include tests to ensure that the behaviour is the desired one.

[paracycle]

I see that this is common to almost all of the commands, so I guess it is better to pull it up to a class_option like --config instead of repeating it for each command.

[Morriar]

While this is defined on most commands, this is not used by all commands. But we could indeed make it available to all so I'm not sure.

[Morriar]

As I said in my comment above, I think this will end up changing what is printed inside the generated file headers as well, and I am not sure if we want that again.

I think that in this particular case this is actually what we want.

With Rails contexts, we're enabling contexts to own their own sets of DSL. Tapioca needs to be ran with RAILS_CONTEXT=my_context bin/tapioca dsl to generate the proper set. This should be written in the headers of the generated files as well.

we used to have this feature and then we removed it

That explains why I could swear we had a similar option but couldn't find it 😂

[kmcphillips]

I think this is correct. Even though it changes what is generated, that is a good thing. I'm trying here to make it such that it's clear for developers how to use default tools to do exactly what they need to do, and having the correct command override in the header is what we want.

[paracycle]

As I said above, I understand the need but we've found that resolving it this way resulted in other problems, historically.

I would like to take a minute and see if we can find another way to do this. I am thinking that if a --config flag is supplied that we could always append it to the generated RBI file headers (and show it in the error output), which would allow us to encapsulate all of these options inside a custom config file.

On top of that, we could also add the ability to set env variables via Tapioca flags, so that that can also be encapsulated in the config as well.

So something like:

$ bin/tapioca dsl --config=sorbet/tapioca/essentials.yml --verify
Loading DSL extension classes... Done
Loading Rails application... Done
Loading DSL compiler classes... Done
Checking for out-of-date RBIs...

RBI files are out-of-date. In your development environment, please run:
 `bin/tapioca dsl --config=sorbet/tapioca/essentials.yml`

with the sorbet/tapioca/essentials.yml file with the contents:

dsl:
  env: 
    RAILS_CONTEXT: "essentials"
  workers: 1
  out: sorbet/contexts/essentials/dsl

[kmcphillips]

We did consider the config flag first. The issue here is that it also depends on an ENV var being set. Which is tricky because that's not technically part of the command or args, but is required. So it doesn't really get included or passed through.

[paracycle]

We did consider the config flag first. The issue here is that it also depends on an ENV var being set. Which is tricky because that's not technically part of the command or args, but is required. So it doesn't really get included or passed through.

That's part of what I wrote above:

On top of that, we could also add the ability to set env variables via Tapioca flags, so that that can also be encapsulated in the config as well.

[Morriar]

I like the config idea though we're very early in our experimentation with contexts, this seems like a lot of investments for a DX we haven't proofed yet.

Let's start with this small change. See how the DX goes with contexts and multiple sets of DSLs, then based on feedback improve the Tapioca implementation.

[paracycle]

I like the config idea though we're very early in our experimentation with contexts, this seems like a lot of investments for a DX we haven't proofed yet.

The change is not complicated at all: #1957

We haven't proven the DX of contexts maybe, but we have a proven DX failure with passing custom commands via command line flags.

Let's start with this small change. See how the DX goes with contexts and multiple sets of DSLs, then based on feedback improve the Tapioca implementation.

The alternative change isn't that big.

[Morriar]

We have a proven DX failure with passing custom commands via command line flags.

In the case of contexts I believe not changing the generated file header will actually be a DX failure.

Let's say you have your main app and the foo context. Here's the structure of the sorbet/ directory:

sorbet/
  |- config # main app config
  |- context/
  |  |- foo/
  |  |  |- config # foo context config
  |  |  |- rbi/
  |  |  |  |- dsl/ # foo context DSL RBIs
  |  |  |  |  |- some_dsl_file.rbi
  |- rbi
  |  |- dsl/ # main app DSL RBIs
  |  |  |- some_dsl_file.rbi

In the case of sorbet/context/foo/rbi/dsl/some_dsl_file.rbi the header should reflect the right way to call Tapioca so the developer can regenerate the file properly.

Without changing the command string, here's the header we're getting:

# DO NOT EDIT MANUALLY
# This is an autogenerated file for dynamic methods in `AbstractController::Caching`.
# Please instead update this file by running `bin/tapioca dsl AbstractController::Caching`.

Running the suggested command will actually regenerate sorbet/rbi/dsl/some_dsl_file.rbi. The header should provide the right command to regenerate this file that may take different options and envs.

[paracycle]

Running the suggested command will actually regenerate sorbet/rbi/dsl/some_dsl_file.rbi. The header should provide the right command to regenerate this file that may take different options and envs.

Maybe there is something wrong with the way I've written my comments above, since it seems like we are talking past each other.

I was NOT denying that this functionality is needed, on the contrary I recognize the need. However, I was trying to find a more limited scope for it. Historically, when we gave folks this kind of general power, they ended up using it for making the command in the headers be stuff like dev rbi gems which stopped working when we started working in Spin, since dev wasn't available, etc. That's why I am trying to make this kind of header change as limited as possible, while allowing the feature you want.

What I suggested above was:

I am thinking that if a --config flag is supplied that we could always append it to the generated RBI file headers (and show it in the error output)

and

On top of that, we could also add the ability to set env variables via Tapioca flags, so that that can also be encapsulated in the config as well.

With both of those in place when you run bin/tapioca dsl --config=sorbet/tapioca/essentials.yml it will use the env variables set in the custom config file for you, and will generate the header of the file as:

# DO NOT EDIT MANUALLY
# This is an autogenerated file for dynamic methods in `AbstractController::Caching`.
# Please instead update this file by running `bin/tapioca dsl --config=sorbet/tapioca/essentials.yml AbstractController::Caching`.

I feel like this limits the scope of this kind of header manipulation as much as possible, since it doesn't allow folks to start supplying anything they want for a --default-command flag that we might add.

[kmcphillips]

We are going to take a different approach, so we won't need this change any more. I'll close it since I won't introduce features that aren't used.