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
Adds Rails::Command::NotesCommand
and makes rake notes
use it under the hood
#33220
Changes from all commits
1edc726
1996fbe
21f7dad
edf5da4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
# frozen_string_literal: true | ||
|
||
require "rails/source_annotation_extractor" | ||
|
||
module Rails | ||
module Command | ||
class NotesCommand < Base # :nodoc: | ||
class_option :annotations, aliases: "-a", desc: "Filter by specific annotations, e.g. Foobar TODO", type: :array, default: %w(OPTIMIZE FIXME TODO) | ||
|
||
def perform(*) | ||
require_application_and_environment! | ||
|
||
deprecation_warning | ||
display_annotations | ||
end | ||
|
||
private | ||
def display_annotations | ||
annotations = options[:annotations] | ||
tag = (annotations.length > 1) | ||
|
||
Rails::SourceAnnotationExtractor.enumerate annotations.join("|"), tag: tag, dirs: directories | ||
end | ||
|
||
def directories | ||
Rails::SourceAnnotationExtractor::Annotation.directories + source_annotation_directories | ||
end | ||
|
||
def deprecation_warning | ||
return if source_annotation_directories.empty? | ||
ActiveSupport::Deprecation.warn("`SOURCE_ANNOTATION_DIRECTORIES` will be deprecated in Rails 6.1. You can add default directories by using config.annotations.register_directories instead.") | ||
end | ||
|
||
def source_annotation_directories | ||
ENV["SOURCE_ANNOTATION_DIRECTORIES"].to_s.split(",") | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,21 +2,21 @@ | |
|
||
require "rails/source_annotation_extractor" | ||
|
||
desc "Enumerate all annotations (use notes:optimize, :fixme, :todo for focus)" | ||
task :notes do | ||
Rails::SourceAnnotationExtractor.enumerate "OPTIMIZE|FIXME|TODO", tag: true | ||
Rails::SourceAnnotationExtractor::Annotation.notes_task_deprecation_warning | ||
Rails::Command.invoke :notes | ||
end | ||
|
||
namespace :notes do | ||
["OPTIMIZE", "FIXME", "TODO"].each do |annotation| | ||
# desc "Enumerate all #{annotation} annotations" | ||
task annotation.downcase.intern do | ||
Rails::SourceAnnotationExtractor.enumerate annotation | ||
Rails::SourceAnnotationExtractor::Annotation.notes_task_deprecation_warning | ||
Rails::Command.invoke :notes, ["--annotations", annotation] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the future we'd want to the command infrastructure to support: Reverting back to a CLI-like argument passing is ridiculous when we're already in Ruby 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I'll look into adding support for it in a follow up PR. Any reasons why it doesn't support that already? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We haven't had a lot of command invocations from the Ruby side internally in Rails yet, so I just never got to it. But when we eventually want to expose commands to apps, running them from other commands in a nicer way is the way to go. |
||
end | ||
end | ||
|
||
desc "Enumerate a custom annotation, specify with ANNOTATION=CUSTOM" | ||
task :custom do | ||
Rails::SourceAnnotationExtractor.enumerate ENV["ANNOTATION"] | ||
Rails::SourceAnnotationExtractor::Annotation.notes_task_deprecation_warning | ||
Rails::Command.invoke :notes, ["--annotations", ENV["ANNOTATION"]] | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,128 @@ | ||
# frozen_string_literal: true | ||
|
||
require "isolation/abstract_unit" | ||
require "rails/command" | ||
require "rails/commands/notes/notes_command" | ||
|
||
class Rails::Command::NotesTest < ActiveSupport::TestCase | ||
setup :build_app | ||
teardown :teardown_app | ||
|
||
test "`rails notes` displays results for default directories and default annotations" do | ||
app_file "app/controllers/some_controller.rb", "# OPTIMIZE: note in app directory" | ||
app_file "config/initializers/some_initializer.rb", "# TODO: note in config directory" | ||
app_file "db/some_seeds.rb", "# FIXME: note in db directory" | ||
app_file "lib/some_file.rb", "# TODO: note in lib directory" | ||
app_file "test/some_test.rb", 1000.times.map { "" }.join("\n") << "# FIXME: note in test directory" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got curious here and experimented in my console. So this works app_file "test/some_test.rb", "\n" * 1000 << "# FIXME: note in test directory" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. I just copied some test cases from the |
||
|
||
app_file "some_other_dir/blah.rb", "# TODO: note in some_other directory" | ||
|
||
assert_equal <<~OUTPUT, run_notes_command | ||
app/controllers/some_controller.rb: | ||
* [ 1] [OPTIMIZE] note in app directory | ||
|
||
config/initializers/some_initializer.rb: | ||
* [ 1] [TODO] note in config directory | ||
|
||
db/some_seeds.rb: | ||
* [ 1] [FIXME] note in db directory | ||
|
||
lib/some_file.rb: | ||
* [ 1] [TODO] note in lib directory | ||
|
||
test/some_test.rb: | ||
* [1000] [FIXME] note in test directory | ||
|
||
OUTPUT | ||
end | ||
|
||
test "`rails notes` displays an empty array when no results were found" do | ||
assert_equal "", run_notes_command | ||
end | ||
|
||
test "`rails notes --annotations` displays results for a single annotation without being prefixed by a tag" do | ||
app_file "db/some_seeds.rb", "# FIXME: note in db directory" | ||
app_file "test/some_test.rb", 1000.times.map { "" }.join("\n") << "# FIXME: note in test directory" | ||
|
||
app_file "app/controllers/some_controller.rb", "# OPTIMIZE: note in app directory" | ||
app_file "config/initializers/some_initializer.rb", "# TODO: note in config directory" | ||
|
||
assert_equal <<~OUTPUT, run_notes_command(["--annotations", "FIXME"]) | ||
db/some_seeds.rb: | ||
* [ 1] note in db directory | ||
|
||
test/some_test.rb: | ||
* [1000] note in test directory | ||
|
||
OUTPUT | ||
end | ||
|
||
test "`rails notes --annotations` displays results for multiple annotations being prefixed by a tag" do | ||
app_file "app/controllers/some_controller.rb", "# FOOBAR: note in app directory" | ||
app_file "config/initializers/some_initializer.rb", "# TODO: note in config directory" | ||
app_file "lib/some_file.rb", "# TODO: note in lib directory" | ||
|
||
app_file "test/some_test.rb", 1000.times.map { "" }.join("\n") << "# FIXME: note in test directory" | ||
|
||
assert_equal <<~OUTPUT, run_notes_command(["--annotations", "FOOBAR", "TODO"]) | ||
app/controllers/some_controller.rb: | ||
* [1] [FOOBAR] note in app directory | ||
|
||
config/initializers/some_initializer.rb: | ||
* [1] [TODO] note in config directory | ||
|
||
lib/some_file.rb: | ||
* [1] [TODO] note in lib directory | ||
|
||
OUTPUT | ||
end | ||
|
||
test "displays results from additional directories added to the default directories from a config file" do | ||
app_file "db/some_seeds.rb", "# FIXME: note in db directory" | ||
app_file "lib/some_file.rb", "# TODO: note in lib directory" | ||
app_file "spec/spec_helper.rb", "# TODO: note in spec" | ||
app_file "spec/models/user_spec.rb", "# TODO: note in model spec" | ||
|
||
add_to_config "config.annotations.register_directories \"spec\"" | ||
|
||
assert_equal <<~OUTPUT, run_notes_command | ||
db/some_seeds.rb: | ||
* [1] [FIXME] note in db directory | ||
|
||
lib/some_file.rb: | ||
* [1] [TODO] note in lib directory | ||
|
||
spec/models/user_spec.rb: | ||
* [1] [TODO] note in model spec | ||
|
||
spec/spec_helper.rb: | ||
* [1] [TODO] note in spec | ||
|
||
OUTPUT | ||
end | ||
|
||
test "displays results from additional file extensions added to the default extensions from a config file" do | ||
add_to_config "config.assets.precompile = []" | ||
add_to_config %q{ config.annotations.register_extensions("scss", "sass") { |annotation| /\/\/\s*(#{annotation}):?\s*(.*)$/ } } | ||
app_file "db/some_seeds.rb", "# FIXME: note in db directory" | ||
app_file "app/assets/stylesheets/application.css.scss", "// TODO: note in scss" | ||
app_file "app/assets/stylesheets/application.css.sass", "// TODO: note in sass" | ||
|
||
assert_equal <<~OUTPUT, run_notes_command | ||
app/assets/stylesheets/application.css.sass: | ||
* [1] [TODO] note in sass | ||
|
||
app/assets/stylesheets/application.css.scss: | ||
* [1] [TODO] note in scss | ||
|
||
db/some_seeds.rb: | ||
* [1] [FIXME] note in db directory | ||
|
||
OUTPUT | ||
end | ||
|
||
private | ||
def run_notes_command(args = []) | ||
rails "notes", args | ||
end | ||
end |
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.
@kaspth @rafaelfranca I realized I can't use
say
here or it ends up printing the result of that method which is an array of all the files on top of the desired output which is coming from a bunch ofputs
inside that module. This is not optimal as I'm outputting from a bunch of different places.I really feel like we should only allow one way to output from a
Rails::Command
. Having this mix of ways to output could end up biting us later if we wanted to output the result in another place thanstdout
. It also bypasses the formatting options coming from Thor'ssay
.I don't think it's worth bloating this PR more than it already is but do you think it would be worth having a follow up PR making
enumerate
return a string we can output withsay
here or are we ok with having outputs coming from bothputs
andsay
across different modules?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.
I think having
puts
littered throughout a helper class is because Rake might be cumbersome to communicate back and forth with (or it just doesn't feel nice to do so).I like to think of commands as similar to controllers. Where controllers are traffic cops for HTTP params and how to respond over HTTP (or whatever), commands are traffic cops for IO in/out and more in that CLI vein. The latest exploration of that was in the server command, where we moved some printing out of Rails::Server and into the command.
Perhaps that might be too restrictive. Perhaps there's a worthy experiment in seeing what executing a command within a context where
puts
andprint
are overriden, so we can capture whatever a helper object will output. Otherwise we might have to pass an IO object around everywhere.I find
say
annoying enough as is. It's too close toputs
to seem worth writing it, so perhaps we should rename it to something more context appropriate.There could perhaps also be something about doing
ActiveSupport::Notifications
so you could do log subscribing on commands.Perhaps. There's definitely something about how it's best to communicate between a command and its collaborator objects that could be teased out here.