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

UnrecognizedCommandError can be corrected and retried #51502

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andrewn617
Copy link
Contributor

@andrewn617 andrewn617 commented Apr 5, 2024

Motivation / Background

This is a follow up to #50941. "Did you mean?" style errors were introduced in 2530160 for unrecognized commands. With this change, we give the user the option to retry with the corrected command name - instead of needing to retype and rerun the command.

Detail

We iterate through all the possible commands returned by DidYouMean::SpellChecker and give the user the option to rerun the suggested correction.

 ➜  myapp git:(main) bin/rails vershen
Unrecognized command "vershen"

Did you mean?  version [Yn] y
Rails 7.2.0.alpha

Additional information

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the railties label Apr 5, 2024
@andrewn617 andrewn617 force-pushed the correct-command-errors branch 2 times, most recently from 6ee9342 to 81a2461 Compare April 5, 2024 15:25
Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

Nice, I like this. Left a comment about changelog wording.

Comment on lines 1 to 2
* When encountering an Unrecognized Command Error, the developer will be given the option to
run of the suggested corrections instead.
Copy link
Member

Choose a reason for hiding this comment

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

I found the wording a little confusing. How about:

Suggested change
* When encountering an Unrecognized Command Error, the developer will be given the option to
run of the suggested corrections instead.
* When encountering an Unrecognized Command Error while running `rails` commands, the developer will be
given the option to run the suggested corrections.

Copy link
Contributor Author

@andrewn617 andrewn617 Apr 5, 2024

Choose a reason for hiding this comment

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

Thanks for the suggestion! I sometimes find writing these change logs harder than writing the code 😅

When encountering a UnrecognizedCommandError, the developer will be given the option to run of the suggested corrections instead.

Co-authored-by: Gannon McGibbon <gannon.mcgibbon@gmail.com>
@p8
Copy link
Member

p8 commented Apr 5, 2024

Nice change!
Does this always return a single suggestion of could there be multiple?

 ➜  myapp git:(main) bin/rails active_text:install
Unrecognized command "active_text:install"

Did you mean?
1. action_text:install
2. active_storage:install

@andrewn617
Copy link
Contributor Author

andrewn617 commented Apr 5, 2024

@p8 Glad you like it :D

Yes, you could get more than one correction. In which case you would get prompted for the first, if you say no, then you'll get prompted for the next one, and so on. The output is like:

Screenshot 2024-04-05 at 5 20 30 PM

In reality I wonder how common that is though - this example is somewhat contrived as a real user probably would have said "yes" to activestorage:install - the first suggestion.

I do like the idea of seeing all the options and selecting them by number. Maybe I can go back and look at how to accomplish that with thor.

@matthewd
Copy link
Member

matthewd commented Apr 5, 2024

I'm not a fan of turning a non-interactive command into an interactive one on the failure path. It interferes with usual CLI error handling, and risks hanging the process -- the tty check is useful, but I'm pretty sure it's not perfect.

Suggesting action text vs active storage seems like an illustration of when it's not helpful, too: if our suggestion isn't pretty close to what they asked for, then the problem likely lies elsewhere (they spelled it correctly, but need to add a gem first). Potentially making multiple implausible guesses, one at a time, when they can immediately see what they actually need to do next, feels hostile.

@gmcgibbon
Copy link
Member

the tty check is useful, but I'm pretty sure it's not perfect.

We spent awhile trying to circumvent the tty? check and couldn't find a way to make it prompt without being attached to a real tty. The implementation uses the os method isatty, so it seems pretty solid to me. Tough, there may be use-cases we're not thinking of, that's true. Without a solid use-case, I wonder if it is the developer's fault at that point or ours. How do you feel about adding an env var to override the behaviour if it breaks?

Suggesting action text vs active storage seems like an illustration of when it's not helpful, too

True. Maybe we could limit the suggestion to one correction, at least limit it to 3 at most.

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

5 participants