-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Load Rake tasks only once for command suggestions #47718
Merged
jonathanhefner
merged 4 commits into
rails:main
from
jonathanhefner:command-did_you_mean-load-rake-tasks-only-once
Mar 28, 2023
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
078a1d6
Split up railties/test/command/application_test.rb
jonathanhefner a32ff0e
Exclude application Rake tasks from help
jonathanhefner b368389
Refactor Rails::Commands::RakeCommand
jonathanhefner 455b8e4
Load Rake tasks only once for command suggestions
jonathanhefner File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
# frozen_string_literal: true | ||
|
||
require "isolation/abstract_unit" | ||
require "rails/command" | ||
|
||
class Rails::Command::HelpIntegrationTest < ActiveSupport::TestCase | ||
setup :build_app | ||
teardown :teardown_app | ||
|
||
test "prints helpful error on unrecognized command" do | ||
output = rails "vershen", allow_failure: true | ||
|
||
assert_match %(Unrecognized command "vershen"), output | ||
assert_match "Did you mean? version", output | ||
end | ||
|
||
test "loads Rake tasks only once on unrecognized command" do | ||
app_file "lib/tasks/my_task.rake", <<~RUBY | ||
puts "MY_TASK already defined? => \#{!!defined?(MY_TASK)}" | ||
MY_TASK = true | ||
RUBY | ||
|
||
output = rails "vershen", allow_failure: true | ||
|
||
assert_match "MY_TASK already defined? => false", output | ||
assert_no_match "MY_TASK already defined? => true", output | ||
end | ||
|
||
test "prints help via `X:help` command when running `X` and `X:X` command is not defined" do | ||
help = rails "dev:help" | ||
output = rails "dev", allow_failure: true | ||
|
||
assert_equal help, output | ||
end | ||
|
||
test "excludes application Rake tasks from command listing" do | ||
app_file "Rakefile", <<~RUBY, "a" | ||
desc "my_task" | ||
task :my_task_1 | ||
RUBY | ||
|
||
app_file "lib/tasks/my_task.rake", <<~RUBY | ||
desc "my_task" | ||
task :my_task_2 | ||
RUBY | ||
|
||
assert_no_match "my_task", rails("--help") | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# frozen_string_literal: true | ||
|
||
require "abstract_unit" | ||
require "rails/command" | ||
|
||
class Rails::Command::ApplicationTest < ActiveSupport::TestCase | ||
test "rails new without path prints help" do | ||
output = run_application_command "new" | ||
|
||
# Doesn't include the default thor error message: | ||
assert_not output.start_with?("No value provided for required arguments") | ||
|
||
# Includes contents of ~/railties/lib/rails/generators/rails/app/USAGE: | ||
assert output.include?("The `rails new` command creates a new Rails application with a default | ||
directory structure and configuration at the path you specify.") | ||
end | ||
|
||
private | ||
def run_application_command(*args) | ||
capture(:stdout) { Rails::Command.invoke(:application, args) } | ||
end | ||
end |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two different paths to set a memoized value makes me nervous, especially given the sequence of steps involved.
Any way we can unify them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primary difference between the two paths is the way the Rake tasks are loaded. This difference dates back to the original implementation:
rails/railties/lib/rails/commands/rake/rake_command.rb
Lines 11 to 19 in 6813edc
rails/railties/lib/rails/commands/rake/rake_command.rb
Lines 24 to 36 in 6813edc
I'm not 100% sure why it was written that way. My guess is the purpose was to exclude tasks defined in the application's
Rakefile
from being listed in the help output. But that would not exclude tasks defined inlib/tasks/*.rake
.Other than that, the two paths end up being equivalent for the default generated
Rakefile
.RakeCommand::perform
callsRake::Application#load_rakefile
, which then loads the app and callsRails.application.load_tasks
:rails/railties/lib/rails/generators/rails/app/templates/Rakefile.tt
Lines 4 to 6 in ba19dbc
RakeCommand::rake_tasks
loads the app:rails/railties/lib/rails/command/actions.rb
Lines 13 to 16 in ba19dbc
and calls
Rails.application.load_tasks
:rails/railties/lib/rails/command/actions.rb
Lines 42 to 44 in ba19dbc
And for an engine, it just calls
Rake::Application#load_rakefile
directly:rails/railties/lib/rails/command/actions.rb
Lines 31 to 34 in ba19dbc
That being said, we can exclude tasks from the help by using
Rake::Task#locations
instead. Doing so also allows us to filter out tasks defined by the application inlib/tasks/*.rake
.I also think it makes sense to load tasks in the same way in both
::perform
and::rake_tasks
. That way::rake_tasks
only lists tasks which can actually be run by::perform
(in case those lists were to diverge somehow).So I've added a couple of commits to make those changes. However, I've kept
@rake_tasks
in::perform
because I think it fits conceptually. Basically,::perform
is preserving its tasks before theRake::Application
instance is thrown away. Then, if any tasks have been preserved,::rake_tasks
returns them; otherwise,::rake_tasks
loads tasks in the same way that::perform
would and returns those.