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

Adds `Rails::Command::NotesCommand` and makes `rake notes` use it under the hood #33220

Merged
merged 4 commits into from Jul 5, 2018
Merged
Diff settings

Always

Just for now

@@ -457,65 +457,90 @@ More information about migrations can be found in the [Migrations](active_record
### `notes`
`bin/rails notes` will search through your code for comments beginning with FIXME, OPTIMIZE, or TODO. The search is done in files with extension `.builder`, `.rb`, `.rake`, `.yml`, `.yaml`, `.ruby`, `.css`, `.js`, and `.erb` for both default and custom annotations.
`bin/rails notes` searches through your code for comments beginning with a specific keyword. You can refer to `bin/rails notes --help` for information about usage.
By default, it will search in `app`, `config`, `db`, `lib`, and `test` directories for FIXME, OPTIMIZE, and TODO annotations in files with extension `.builder`, `.rb`, `.rake`, `.yml`, `.yaml`, `.ruby`, `.css`, `.js`, and `.erb`.
```bash
$ bin/rails notes
(in /home/foobar/commandsapp)
app/controllers/admin/users_controller.rb:
* [ 20] [TODO] any other way to do this?
* [132] [FIXME] high priority for next deploy
app/models/school.rb:
lib/school.rb:
* [ 13] [OPTIMIZE] refactor this code to make it faster
* [ 17] [FIXME]
```
You can add support for new file extensions using `config.annotations.register_extensions` option, which receives a list of the extensions with its corresponding regex to match it up.
```ruby
config.annotations.register_extensions("scss", "sass", "less") { |annotation| /\/\/\s*(#{annotation}):?\s*(.*)$/ }
```
#### Annotations
If you are looking for a specific annotation, say FIXME, you can use `bin/rails notes:fixme`. Note that you have to lower case the annotation's name.
You can pass specific annotations by using the `--annotations` argument. By default, it will search for FIXME, OPTIMIZE, and TODO.
Note that annotations are case sensitive.
```bash
$ bin/rails notes:fixme
(in /home/foobar/commandsapp)
$ bin/rails notes --annotations FIXME RELEASE
app/controllers/admin/users_controller.rb:
* [132] high priority for next deploy
* [101] [RELEASE] We need to look at this before next release
* [132] [FIXME] high priority for next deploy
app/models/school.rb:
* [ 17]
lib/school.rb:
* [ 17] [FIXME]
```
You can also use custom annotations in your code and list them using `bin/rails notes:custom` by specifying the annotation using an environment variable `ANNOTATION`.
#### Directories
You can add more default directories to search from by using `config.annotations.register_directories`. It receives a list of directory names.
```ruby
config.annotations.register_directories("spec", "vendor")
```
```bash
$ bin/rails notes:custom ANNOTATION=BUG
(in /home/foobar/commandsapp)
app/models/article.rb:
* [ 23] Have to fix this one before pushing!
$ bin/rails notes
app/controllers/admin/users_controller.rb:
* [ 20] [TODO] any other way to do this?
* [132] [FIXME] high priority for next deploy
lib/school.rb:
* [ 13] [OPTIMIZE] Refactor this code to make it faster
* [ 17] [FIXME]
spec/models/user_spec.rb:
* [122] [TODO] Verify the user that has a subscription works
vendor/tools.rb:
* [ 56] [TODO] Get rid of this dependency
```
NOTE. When using specific annotations and custom annotations, the annotation name (FIXME, BUG etc) is not displayed in the output lines.
#### Extensions
By default, `rails notes` will look in the `app`, `config`, `db`, `lib`, and `test` directories. If you would like to search other directories, you can configure them using `config.annotations.register_directories` option.
You can add more default file extensions to search from by using `config.annotations.register_extensions`. It receives a list of extensions with its corresponding regex to match it up.
```ruby
config.annotations.register_directories("spec", "vendor")
config.annotations.register_extensions("scss", "sass") { |annotation| /\/\/\s*(#{annotation}):?\s*(.*)$/ }
```
You can also provide them as a comma separated list in the environment variable `SOURCE_ANNOTATION_DIRECTORIES`.
```bash
$ export SOURCE_ANNOTATION_DIRECTORIES='spec,vendor'
$ bin/rails notes
(in /home/foobar/commandsapp)
app/models/user.rb:
* [ 35] [FIXME] User should have a subscription at this point
app/controllers/admin/users_controller.rb:
* [ 20] [TODO] any other way to do this?
* [132] [FIXME] high priority for next deploy
app/assets/stylesheets/application.css.sass:
* [ 34] [TODO] Use pseudo element for this class
app/assets/stylesheets/application.css.scss:
* [ 1] [TODO] Split into multiple components
lib/school.rb:
* [ 13] [OPTIMIZE] Refactor this code to make it faster
* [ 17] [FIXME]
spec/models/user_spec.rb:
* [122] [TODO] Verify the user that has a subscription works
vendor/tools.rb:
* [ 56] [TODO] Get rid of this dependency
```
### `routes`
@@ -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

This comment has been minimized.

Copy link
@anniecodes

anniecodes Jul 4, 2018

Author Contributor

@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 of puts 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 than stdout. It also bypasses the formatting options coming from Thor's say.

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 with say here or are we ok with having outputs coming from both puts and say across different modules?

This comment has been minimized.

Copy link
@kaspth

kaspth Jul 4, 2018

Member

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.

I really feel like we should only allow one way to output from a Rails::Command.

Perhaps that might be too restrictive. Perhaps there's a worthy experiment in seeing what executing a command within a context where puts and print 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 to puts 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.

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 with say here or are we ok with having outputs coming from both puts and say across different modules?

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.

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
@@ -8,12 +8,7 @@
new("SourceAnnotationExtractor", "Rails::SourceAnnotationExtractor")

module Rails
# Implements the logic behind the rake tasks for annotations like
#
# rails notes
# rails notes:optimize
#
# and friends. See <tt>rails -T notes</tt> and <tt>railties/lib/rails/tasks/annotations.rake</tt>.
# Implements the logic behind <tt>Rails::Command::NotesCommand</tt>. See <tt>rails notes --help</tt> for usage information.
#
# Annotation objects are triplets <tt>:line</tt>, <tt>:tag</tt>, <tt>:text</tt> that
# represent the line where the annotation lives, its tag, and its text. Note
@@ -25,7 +20,7 @@ module Rails
class SourceAnnotationExtractor
class Annotation < Struct.new(:line, :tag, :text)
def self.directories
@@directories ||= %w(app config db lib test) + (ENV["SOURCE_ANNOTATION_DIRECTORIES"] || "").split(",")
@@directories ||= %w(app config db lib test)
end

# Registers additional directories to be included
@@ -59,23 +54,27 @@ def to_s(options = {})
s << "[#{tag}] " if options[:tag]
s << text
end

# Used in annotations.rake
#:nodoc:
def self.notes_task_deprecation_warning
ActiveSupport::Deprecation.warn("This rake task is deprecated and will be removed in Rails 6.1. \nRefer to `rails notes --help` for more information.\n")
puts "\n"
end
end

# Prints all annotations with tag +tag+ under the root directories +app+,
# +config+, +db+, +lib+, and +test+ (recursively).
#
# Additional directories may be added using a comma-delimited list set using
# <tt>ENV['SOURCE_ANNOTATION_DIRECTORIES']</tt>.
#
# Directories may also be explicitly set using the <tt>:dirs</tt> key in +options+.
# Specific directories can be explicitly set using the <tt>:dirs</tt> key in +options+.
#
# Rails::SourceAnnotationExtractor.enumerate 'TODO|FIXME', dirs: %w(app lib), tag: true
#
# If +options+ has a <tt>:tag</tt> flag, it will be passed to each annotation's +to_s+.
#
# See <tt>#find_in</tt> for a list of file extensions that will be taken into account.
#
# This class method is the single entry point for the rake tasks.
# This class method is the single entry point for the `rails notes` command.
def self.enumerate(tag, options = {})
extractor = new(tag)
dirs = options.delete(:dirs) || Annotation.directories
@@ -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]

This comment has been minimized.

Copy link
@kaspth

kaspth Jul 1, 2018

Member

In the future we'd want to the command infrastructure to support: Rails::Command.invoke :notes, annotations: annotation

Reverting back to a CLI-like argument passing is ridiculous when we're already in Ruby 😄

This comment has been minimized.

Copy link
@anniecodes

anniecodes Jul 3, 2018

Author Contributor

Agreed. I'll look into adding support for it in a follow up PR. Any reasons why it doesn't support that already?

This comment has been minimized.

Copy link
@kaspth

kaspth Jul 4, 2018

Member

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
@@ -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"

This comment has been minimized.

Copy link
@kaspth

kaspth Jul 5, 2018

Member

Got curious here and experimented in my console. 1000.times.map { "" }.join("\n") could be cut down to just "\n" * 1000.

So this works

app_file "test/some_test.rb", "\n" * 1000 << "# FIXME: note in test directory"

This comment has been minimized.

Copy link
@anniecodes

anniecodes Jul 5, 2018

Author Contributor

You're right. I just copied some test cases from the rake notes tests 😅. To be real I could also cut it down to just a 2 or 3 digits number since it's mostly for testing the indentation/alignment of the line number as well as the annotation parsing more than one line per file. I'll put up a quick PR for it since it's a little overkill right now.


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
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.