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

Move all Sorbet related commands as subcommands #524

Merged
merged 3 commits into from Mar 12, 2024
Merged

Conversation

Morriar
Copy link
Collaborator

@Morriar Morriar commented Mar 6, 2024

Currently, all the commands are top level making both the code and DX a bit messy. I also want to make some space for other commands such as the deadcode related ones.

Those are the commands currently merged:

  spoom bump            # Bump Sorbet sigils from `false` to `true` when no errors
  spoom coverage        # Collect metrics related to Sorbet coverage
  spoom lsp             # Send LSP requests to Sorbet
  spoom tc              # Run Sorbet and parses its output

With this change, all commands related to Sorbet are moved under the parent command srb:

  spoom srb bump            # Bump Sorbet sigils from `false` to `true` when no errors
  spoom srb coverage        # Collect metrics related to Sorbet coverage
  spoom srb lsp             # Send LSP requests to Sorbet
  spoom srb tc              # Run Sorbet and parses its output

To avoid breaking right now, I kept the top level commands to redirect to the srb ones with a deprecation message:

$ bundle exec spoom tc
Warning: This command is deprecated. Please use spoom srb tc instead.

We can remove them in the later major version bump.

@Morriar Morriar added the chore Chore task label Mar 6, 2024
@Morriar Morriar self-assigned this Mar 6, 2024
@Morriar Morriar requested a review from a team as a code owner March 6, 2024 20:15
option :sorbet_options, type: :string, default: "", desc: "Pass options to Sorbet"
sig { params(directory: String).void }
def bump(directory = ".")
say_warning("This command is deprecated. Please use `spoom srb bump` instead.")
Copy link
Contributor

@andyw8 andyw8 Mar 7, 2024

Choose a reason for hiding this comment

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

People may wonder if the new one is the same or not. Would it be better to say it's been 'renamed' or 'moved'? Or we could append, "...instead, which has identical behavior".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you think the release notes would be enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, but I suspect few people read them.

Copy link
Contributor

@andyw8 andyw8 left a comment

Choose a reason for hiding this comment

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

(my comment is non-blocking)

Still keep an redirection from old commands for now but display a deprecation warning.

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar Morriar enabled auto-merge March 12, 2024 14:12
@Morriar Morriar merged commit cbb9789 into main Mar 12, 2024
8 checks passed
@Morriar Morriar deleted the at-deprecate-commands branch March 12, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Chore task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants