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

Add "Did you mean?" for unrecognized CLI commands #47208

Merged
merged 1 commit into from Jan 31, 2023

Conversation

jonathanhefner
Copy link
Member

This commit improves the error message that is displayed when a user specifies an unrecognized bin/rails command by offering "Did you mean?" suggestions. The suggestions cover all visible commands, and supersede any incomplete (Rake-task-only) suggestions that Rake might display.

Before (example)

$ bin/rails credentails:edit
rails aborted!
Don't know how to build task 'credentails:edit' (See the list of available tasks with `rails --tasks`)

(See full trace by running task with --trace)

$ bin/rails --tasks
...
rails cache_digests:dependencies         # Lookup first-level dependencies for TEMPLATE (like messages/show or comments/_comment.html)
rails cache_digests:nested_dependencies  # Lookup nested dependencies for TEMPLATE (like messages/show or comments/_comment.html)
rails db:create                          # Creates the database from DATABASE_URL or config/database.yml for the current RAILS_ENV (use ...
rails db:drop                            # Drops the database from DATABASE_URL or config/database.yml for the current RAILS_ENV (use db...
...

$ bin/rails credentails -h
# no output

$ bin/rails credentials -h
# no output

$ bin/rails versoin
rails aborted!
Don't know how to build task 'versoin' (See the list of available tasks with `rails --tasks`)
Did you mean?  db:version

(See full trace by running task with --trace)

After (example)

$ bin/rails credentails:edit
Unrecognized command "credentails:edit" (Rails::Command::UnrecognizedCommandError)
Did you mean?  credentials:edit

$ bin/rails credentails -h
Unrecognized command "credentails" (Rails::Command::UnrecognizedCommandError)
Did you mean?  credentials:diff

$ bin/rails credentials -h
Unrecognized command "credentials" (Rails::Command::UnrecognizedCommandError)
Did you mean?  credentials:diff

$ bin/rails versoin
Unrecognized command "versoin" (Rails::Command::UnrecognizedCommandError)
Did you mean?  version

@rails-bot rails-bot bot added the railties label Jan 31, 2023
@jonathanhefner jonathanhefner force-pushed the command-did_you_mean branch 2 times, most recently from cea89fc to 4e80029 Compare January 31, 2023 21:21
This commit improves the error message that is displayed when a user
specifies an unrecognized `bin/rails` command by offering "Did you
mean?" suggestions.  The suggestions cover all visible commands, and
supersede any incomplete (Rake-task-only) suggestions that Rake might
display.

__Before (example)__

  ```console
  $ bin/rails credentails:edit
  rails aborted!
  Don't know how to build task 'credentails:edit' (See the list of available tasks with `rails --tasks`)

  (See full trace by running task with --trace)

  $ bin/rails --tasks
  ...
  rails cache_digests:dependencies         # Lookup first-level dependencies for TEMPLATE (like messages/show or comments/_comment.html)
  rails cache_digests:nested_dependencies  # Lookup nested dependencies for TEMPLATE (like messages/show or comments/_comment.html)
  rails db:create                          # Creates the database from DATABASE_URL or config/database.yml for the current RAILS_ENV (use ...
  rails db:drop                            # Drops the database from DATABASE_URL or config/database.yml for the current RAILS_ENV (use db...
  ...

  $ bin/rails credentails -h
  # no output

  $ bin/rails credentials -h
  # no output

  $ bin/rails versoin
  rails aborted!
  Don't know how to build task 'versoin' (See the list of available tasks with `rails --tasks`)
  Did you mean?  db:version

  (See full trace by running task with --trace)
  ```

__After (example)__

  ```console
  $ bin/rails credentails:edit
  Unrecognized command "credentails:edit" (Rails::Command::UnrecognizedCommandError)
  Did you mean?  credentials:edit

  $ bin/rails credentails -h
  Unrecognized command "credentails" (Rails::Command::UnrecognizedCommandError)
  Did you mean?  credentials:diff

  $ bin/rails credentials -h
  Unrecognized command "credentials" (Rails::Command::UnrecognizedCommandError)
  Did you mean?  credentials:diff

  $ bin/rails versoin
  Unrecognized command "versoin" (Rails::Command::UnrecognizedCommandError)
  Did you mean?  version
  ```
@jonathanhefner jonathanhefner merged commit 4d324e3 into rails:main Jan 31, 2023
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Mar 14, 2023
Follow-up to rails#47208.

When using Rake-style CLI arguments, `Rake::Application#top_level_tasks`
will return the full task invocation rather than the parsed task name.
For example, when running `rake foo[bar] baz[qux]`, `top_level_tasks`
will return `["foo[bar]", "baz[qux]"]` rather than `["foo", "baz"]`.
Therefore, we must extract the task name before checking existence.
@etiennebarrie
Copy link
Contributor

etiennebarrie commented Mar 20, 2023

This causes a warning with a default generated engine plugin. It's also related to #46664 (edit: wrong PR).

$ rails plugin new my_engine --main --full
$ cd my_engine
$ sed -Ei '' -e '/uri|homepage|host/ d' -e 's/TODO: //' *.gemspec # fix gemspec to be able to bundle
$ bin/rails test >/dev/null
/tmp/my_engine/Rakefile:3: warning: already initialized constant APP_RAKEFILE
/tmp/my_engine/Rakefile:3: warning: previous definition of APP_RAKEFILE was here

Engine plugins have a curious setup which defines all the dummy app's tasks under the :app namespace:

namespace :app do
load APP_RAKEFILE

Since #47210 we check if there's a test:prepare task:
def run_prepare_task
Rails::Command::RakeCommand.perform("test:prepare", [], {})
rescue UnrecognizedCommandError => error
raise unless error.name == "test:prepare"
end

Even though the exception is rescued, to initialize it we need all the existing commands:

super("Unrecognized command #{name.inspect}", name, Command.printing_commands.map(&:first))

which asks RakeCommand its commands, which loads the Rakefile again:
def load_tasks
Rake.application.init("rails")
Rake.application.load_rakefile
end


I can see multiple ways we can fix this:

  • UnrecognizedCommandError could delay finding the candidates until they're needed for displaying the exception (since we rescue the exception we don't need it to be displayed). That doesn't solve the underlying double-loading issue though.
  • We could re-visit the idea of loading the dummy app's tasks under the :app namespace, I'm not entirely sure why we did and what the consequences would be. For now, that means we don't run test:prepare in engines even if the dummy app has some dependencies that require it.
  • In Command::Actions#load_tasks, in the engine case, we could add a conditional to only load tasks if they don't seem to have been loaded already, maybe simply checking if Rails.application.tasks is not empty.

I think the last one makes the most sense, but we need the metadata since we're filtering the tasks without comments, just doing that would remove the did you mean in that case. I think we could load the metadata in engine.rake, that way we don't lose that information for that specific case.

jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Mar 20, 2023
Follow-up to rails#47208.

Prior to rails#47208, `Rails::Command::ApplicationTest` appears to have been
specifically intended to test `Rails::Command::ApplicationCommand`.
Therefore, this commit extracts the tests that were added by rails#47208 out
from `railties/test/command/application_test.rb` into
`railties/test/command/help_integration_test.rb`, and moves
`application_test.rb` from `railties/test/command` to
`railties/test/commands` (with the rest of the command-specific tests).
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Mar 20, 2023
Follow-up to rails#47208.

`UnrecognizedCommandError` calls `printing_commands` to make "Did you
mean?" suggestions.  `printing_commands` calls `RakeCommand#rake_tasks`,
which loads the Rake tasks if they have not been memoized.  If the tasks
have already been loaded by `RakeCommand#perform` (but not memoized by
`RakeCommand#rake_tasks`), then the tasks will be loaded a second time.
This can cause, for example, constant redefinition warnings if the task
files define constants.

Therefore, this commit memoizes the tasks from `RakeCommand#perform`
before raising `UnrecognizedCommandError`.
@jonathanhefner
Copy link
Member Author

@etiennebarrie Thank you for the detailed write up! 😍

I think avoiding the double load (option 3) is probably the best way. To that end, I've submitted #47718, which memoizes the already-loaded tasks before raising UnrecognizedCommandError.

jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Mar 28, 2023
Follow-up to rails#47208.

Prior to rails#47208, `Rails::Command::ApplicationTest` appears to have been
specifically intended to test `Rails::Command::ApplicationCommand`.
Therefore, this commit extracts the tests that were added by rails#47208 out
from `railties/test/command/application_test.rb` into
`railties/test/command/help_integration_test.rb`, and moves
`application_test.rb` from `railties/test/command` to
`railties/test/commands` (with the rest of the command-specific tests).
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Mar 28, 2023
Follow-up to rails#47208.

`UnrecognizedCommandError` calls `printing_commands` to make "Did you
mean?" suggestions.  `printing_commands` calls `RakeCommand::rake_tasks`,
which loads the Rake tasks if they have not been memoized.  If the tasks
have already been loaded by `RakeCommand::perform` (but not memoized by
`RakeCommand::rake_tasks`), then the tasks will be loaded a second time.
This can cause, for example, constant redefinition warnings if the task
files define constants.

Therefore, this commit memoizes the tasks from `RakeCommand::perform`
before raising `UnrecognizedCommandError`.
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