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

Conversation

Projects
None yet
10 participants
@anniecodes
Copy link
Contributor

anniecodes commented Jun 25, 2018

Summary

This PR adds a new NotesCommand following the pattern for Rails::Commands. rake notes is modified to call that new command under the hood with a deprecation warning when called with the old API.

API Changes

Environment variables used in the rake task are changed to use console options in the new API.

Before After
rails notes rails notes
rails notes:custom ANNOTATION=custom rails notes --annotations custom

Ability to register additional default directories and extensions by using config.annotations.register_directories and config.annotations.register_extensions has been left unchanged.

Backward compatibility / Future deprecation

The commands are fully backward compatible with SOURCE_ANNOTATION_DIRECTORIES=other_folder,spec environment variable but display a deprecation warning for it pointing to using config.annotations.register_directories instead.

The old rails notes:custom, rails notes:fixme, rails notes:todo, rails notes:optimize are marked as deprecated.

Calling any notes command with rake instead of rails is also marked as deprecated

@rails-bot

This comment has been minimized.

Copy link

rails-bot commented Jun 25, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@anniecodes anniecodes force-pushed the anniecodes:notes-command branch Jun 25, 2018

@sikachu
Copy link
Member

sikachu left a comment

I have a few comments, which many of them are just wording improvements. I like the direction of your patch, so I hope my comments won't discourage you from pushing this patch to merge. Thank you very much.

guides/source/command_line.md Outdated
`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`.

This comment has been minimized.

@sikachu

sikachu Jun 26, 2018

Member

Some copywriting suggestion,

  • I think we need a comma after By default

    By default, it will ...
    
  • Please follow Oxford comma rule

    app, config, db, lib, and test directories
    
  • I'm not sure if we need to put those directory name in backticks or not. Do you mind verifying from previous documetations?

This comment has been minimized.

@anniecodes

anniecodes Jun 26, 2018

Author Contributor

Based on http://guides.rubyonrails.org/engines.html app directory seems to be using backticks. (I also am using backticks for the same directory list a few sentences later. Talk about lacking consistency here 😉)

I'll update

guides/source/command_line.md Outdated
```

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 it specific annotations by using the `--annotations` argument. By default, it will search for FIXME, OPTIMIZE, and TODO.

This comment has been minimized.

@sikachu

sikachu Jun 26, 2018

Member

I think the first it is an error here:

You can pass specific annotations
guides/source/command_line.md Outdated
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

It can be passed specific directories in which to execute the search by using the `--directories` argument. By default, it will search in `app`, `config`, `db`, `lib`, and `test` directories.

This comment has been minimized.

@sikachu

sikachu Jun 26, 2018

Member

I don't think this reads very smooth. What do you think about rewriting it to something like

You can limit the search to be within specific directories by using `--directories` argument. By default ...
guides/source/command_line.md Outdated
NOTE. When using specific annotations and custom annotations, the annotation name (FIXME, BUG etc) is not displayed in the output lines.

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 support for additional directories using `config.annotations.register_directories` which receives a list of directory names.

This comment has been minimized.

@sikachu

sikachu Jun 26, 2018

Member

I think we can be more clear that user can add additional directories to the defaults.

You can add more default directories to search from by using `config.annotations.register_directories`
railties/lib/rails/commands/notes/notes_command.rb Outdated
def perform(*)
require_application_and_environment!

annotations = options.fetch(:annotations)

This comment has been minimized.

@sikachu

sikachu Jun 26, 2018

Member

I think fetch is not necessary here? options[:annotations] should be fine since we don't set fallback value.

railties/lib/rails/commands/notes/notes_command.rb Outdated
require_application_and_environment!

annotations = options.fetch(:annotations)
tag = true if annotations.length > 1

This comment has been minimized.

@sikachu

sikachu Jun 26, 2018

Member

We can actually remove trailing if and just set the returning boolean value, I think:

tag = (annotations.length > 1)
railties/lib/rails/commands/notes/notes_command.rb Outdated

annotations = options.fetch(:annotations)
tag = true if annotations.length > 1
directories = options.fetch(:directories, nil)

This comment has been minimized.

@sikachu

sikachu Jun 26, 2018

Member

I believe we can also use options[:directories] here since it will fallback to nil by default.

railties/lib/rails/tasks/annotations.rake Outdated
end
end

def notes_task_deprecation_warning
ActiveSupport::Deprecation.warn("This command is deprecated and will be removed in the next Rails version. \nRefer to `rails notes --help` for more information.\n\n")

This comment has been minimized.

@sikachu

sikachu Jun 26, 2018

Member
  • Do you mind revising the wording a bit?

    This rake task is deprecated and will be removed in the next version of Rails.

  • I'm also not sure about the deprecation timeline for this, so maybe @matthewd or @rafaelfranca can help. I wonder if we should even say "in the next major version of Rails" or something.

railties/lib/rails/tasks/annotations.rake Outdated
end
end

def notes_task_deprecation_warning

This comment has been minimized.

@sikachu

sikachu Jun 26, 2018

Member

I'm pretty sure that this is gong to add the methods on the global namespace, which is probably not the right thing to do. Do you mind moving this into, say, Rails::SourceAnnotationExtractor:: Annotation.display_rake_task_warning or something?

This comment has been minimized.

@anniecodes

anniecodes Jun 26, 2018

Author Contributor

Yes it is added to the global namespace 😢 . That's why I was so specific in my method name but I like your idea of moving it to Rails::SourceAnnotationExtractor:: Annotation. I'll do that 👌

railties/lib/rails/tasks/annotations.rake Outdated
ActiveSupport::Deprecation.warn("This command is deprecated and will be removed in the next Rails version. \nRefer to `rails notes --help` for more information.\n\n")
end

def notes_task_options(additional_options = [])

This comment has been minimized.

@sikachu

sikachu Jun 26, 2018

Member

This method needs to be moved as well.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Jun 26, 2018

rake notes ANNOTATION=custom could currently be spelled rails notes ANNOTATION=custom.

Consequently, I suspect we'll need to teach the new command how to read rake-style options (while showing a deprecation message)... and then the rake task can be a straight pass-through invocation.

@anniecodes anniecodes force-pushed the anniecodes:notes-command branch 2 times, most recently Jun 26, 2018

@anniecodes

This comment has been minimized.

Copy link
Contributor Author

anniecodes commented Jun 26, 2018

@matthewd ANNOTATION=custom can currently only be passed to rails notes:custom not rails notes so since notes:custom doesn't exist in the new rails notes API, it is delegated to the rake task and still works as expected with these changes.

The only place these changes are breaking the current rails notes API is when SOURCE_ANNOTATION_DIRECTORIES is passed to it.

Should I add support for ENV[SOURCE_ANNOTATION_DIRECTORIES] in NotesCommand ?

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Jun 26, 2018

since notes:custom doesn't exist in the new rails notes API, it is delegated to the rake task

Ah, okay, cool.

Should I add support for ENV[SOURCE_ANNOTATION_DIRECTORIES] in NotesCommand ?

A quick GitHub-wide code search says "maybe yes" to me. (Looking at those, widespread use of forms like SOURCE_ANNOTATION_DIRECTORIES=spec has me concerned that the 'Minor added feature' you described may be an unfeature, too, incidentally. What's the use case for wanting to narrow the search so much? Might it be worth a --skip-defaults or something instead? Or, if we're saying the right way to do it is now the config value, do we need a directory option at all?)

railties/lib/rails/source_annotation_extractor.rb Outdated
# Used in annotations.rake
#:nodoc:
def self.notes_task_deprecation_warning
ActiveSupport::Deprecation.warn("This rake task is deprecated and will be removed in the next version of Rails. \nRefer to `rails notes --help` for more information.\n\n")

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 26, 2018

Member

You can put Rails 6.1 here.

@rafaelfranca rafaelfranca assigned rafaelfranca and unassigned sgrif Jun 26, 2018

@anniecodes

This comment has been minimized.

Copy link
Contributor Author

anniecodes commented Jun 26, 2018

widespread use of forms like SOURCE_ANNOTATION_DIRECTORIES=spec

I'll make some changes to make sure SOURCE_ANNOTATION_DIRECTORIES is still supported but with a deprecation warning that points to use register_directories

The thing we need to be aware here is that we lose the ability to dynamically add more directories entirely from the command line which could be considered a regression.

When searching for SOURCE_ANNOTATION_DIRECTORIES= in the entire Github realm, it looks like most people are setting it globally and therefore could have used register_directories so it might be good enough to justify recommending using register_directories in our deprecation warning.

If their use case is not covered by that deprecation warning, we can add an option to NotesCommand for additional_directories that has the same behaviour as register_directories but dynamically or something like that.

What's the use case for wanting to narrow the search so much?

  1. To reflect the SourceAnnotationExtractor::Annotation API through he command line (At first I had options on NotesCommand for directories and additional_directories and extensions to really mirror the capabilities to the user but decided it was a little overkill for a "deprecation" PR 😝 )

  2. I might be biased because of the size of our codebase where usually a team will work in a subset of the codebase and therefore might want to only look at annotations in their specific area without changing the global state of our app with register_directories.

Anyway, I understand it's not a majority case so I'll get rid of directories for now. If we see that it is needed, it's an easy thing to add in the future and will make this PR less meaty 😉

Might it be worth a --skip-defaults or something instead

I'm not sure I understand what the purpose of --skip-defaults would be in this case. Without default it would search in no directories for no annotations which would be pretty pointless. But maybe I'm misunderstanding something :)

@anniecodes anniecodes force-pushed the anniecodes:notes-command branch 7 times, most recently Jun 26, 2018

@anniecodes

This comment has been minimized.

Copy link
Contributor Author

anniecodes commented Jun 26, 2018

I got rid of the --directories option and made sure SOURCE_ANNOTATION_DIRECTORIES was supported for both old and new commands (was already tested in this file for current rails notes command) as well as updates documentation to reflect these changes.

I also updated the description of this PR to reflect the change in approach. It should be good to go now and fully backward compatible.

@nicolas-brousse

This comment has been minimized.

Copy link

nicolas-brousse commented Jun 27, 2018

Hi,

Also what do you think about having the command exit 1 if there is annotations found?
It could be useful in CI.

@anniecodes

This comment has been minimized.

Copy link
Contributor Author

anniecodes commented Jun 27, 2018

@nicolas-brousse My guess it that it's too usage specific to be added to the Rails framework. exit 1 when annotations are found might not apply to everybody depending on their development style. 🤷‍♀️

I think a more generic approach would be to add support for enhancing / hooking into a specific Rails::Command by using some kind of callback in your app where you would be able to have the command exit 1 when annotations are found. Which I think would be a reasonable feature to add.

@kaspth
Copy link
Member

kaspth left a comment

Thanks, @anniecodes! Nice that we'll finally have a command with such great noteriety 😄

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.

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

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

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

railties/test/commands/notes_test.rb Outdated

add_to_config "config.annotations.register_directories \"spec\""

expected_output = [

This comment has been minimized.

@kaspth

kaspth Jul 1, 2018

Member

Would these be clearer with squiggly heredocs?

assert_equal <<~OUTPUT, run_notes_command
  db/some_seeds.rb:


OUTPUT
railties/lib/rails/commands/notes/notes_command.rb Outdated

def source_annotation_directories
ENV["SOURCE_ANNOTATION_DIRECTORIES"]
end

This comment has been minimized.

@kaspth

kaspth Jul 1, 2018

Member

I'd move the split to here (and replace || "" with to_s).

It makes sense that a ENV["SOURCE_ANNOTATION_DIRECTORIES"] returns a string or nil, but it doesn't make sense that a Ruby method named such does. That would be an empty array at worst.

Then display_deprecation_warning can do:

if source_annotation_directories.empty?
  say …
end

This comment has been minimized.

@anniecodes

anniecodes Jul 3, 2018

Author Contributor

I like that. I'll update.

railties/lib/rails/commands/notes/notes_command.rb Outdated
def display_deprecation_warning
return unless source_annotation_directories
say 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.")
say "\n"

This comment has been minimized.

@kaspth

kaspth Jul 1, 2018

Member

Can't remember, do we even need to pass a newline in here? Or would say just do it for us?

This comment has been minimized.

@anniecodes

anniecodes Jul 3, 2018

Author Contributor

You're right. say adds the extra line without need to be passed \n. I'll modify

@anniecodes anniecodes force-pushed the anniecodes:notes-command branch 2 times, most recently Jul 3, 2018

[ci skip] Update documentation related to `rails notes`
* Get rid of references to rake notes in the documentation
* Get rid of references to environement variables used in SourceAnnotationExtractor
* Updates the command line guide to reflect the new rails notes API

@anniecodes anniecodes force-pushed the anniecodes:notes-command branch Jul 4, 2018

annotations = options[:annotations]
tag = (annotations.length > 1)

Rails::SourceAnnotationExtractor.enumerate annotations.join("|"), tag: tag, dirs: directories

This comment has been minimized.

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

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

@sikachu

sikachu approved these changes Jul 4, 2018

Copy link
Member

sikachu left a comment

I think this is good to merge, but will let @rafaelfranca @kaspth pull the trigger.

railties/lib/rails/commands/notes/notes_command.rb Outdated

def display_deprecation_warning
return if source_annotation_directories.empty?
say 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.")

This comment has been minimized.

@Edouard-chin

Edouard-chin Jul 4, 2018

Contributor

Out of curiosity why is the say necessary here? Shouldn't we leave the Deprecation behavior decide what to do when a deprecation is triggered?

This comment has been minimized.

@kaspth

kaspth Jul 4, 2018

Member

True, that's probably not needed. Let's remove it, then I'll merge 👍

This comment has been minimized.

@anniecodes

anniecodes Jul 4, 2018

Author Contributor

If I get rid of the say here, it doesn't output in the console which I don't think is what we want.

This comment has been minimized.

@Edouard-chin

Edouard-chin Jul 4, 2018

Contributor

The user of the app is up to decide whether he wants to output deprecation. (By default it's gonna log in the console, but we shouldn't bypass the setting)

This comment has been minimized.

@anniecodes

anniecodes Jul 4, 2018

Author Contributor

ohhh interesting. TIL. ok I'll make the change then.

anniecodes added some commits Jun 20, 2018

Adds a Rails::Command for Notes
* It is called with `rails notes`
* It defaults to displaying [OPTIMIZE, FIXME and TODO] annotations
* It accepts custom annotations by using `rails notes -a CUSTOM_ANNOTATION OTHER_ANNOTATION`
* It defaults to look for annotations in [app config db lib test] as dictated by SourceAnnotationExtractor
* It supports ENV["SOURCE_ANNOTATION_DIRECTORIES"] but adds a deprecation warning and recommends using register_directories instead
Adds support to register directories and extensions to NotesCommand
* Require the application and environnement in the notes command in order to load the config files
* Adds tests for both register_directories and register_extensions added to a config file
Port Annotations rake task to use Rails::NotesCommand
* Invokes the notes Rails::Command and passes the rake task ENV variables as annotations options to it
* Adds a deprecation warning for unsupported commands
* Gets rid of reference to ENV["SOURCE_ANNOTATION_DIRECTORIES"] in SourceAnnotationExtractor since its now dealt with in the NotesCommand
* Gets rid of rake desc for each rake notes task so they are not documented while using `rails -T` or `rails --help`

@anniecodes anniecodes force-pushed the anniecodes:notes-command branch to edf5da4 Jul 4, 2018

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.

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

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

@kaspth kaspth merged commit ba38e13 into rails:master Jul 5, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Jul 5, 2018

Nice! 💪

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor

bogdanvlviv commented Jul 5, 2018

I think would be great to add changelog entries in railties/CHANGELOG.md about:

  • deprecation using of ENV["SOURCE ANNOTATION DIRECTORIES"]
  • deprecation rake notes in favor of rails notes

anniecodes added a commit to anniecodes/rails that referenced this pull request Jul 9, 2018

Add deprecations related to `rails notes` command to Changelog
* SOURCE_ANNOTATION_DIRECTORIES deprecation
* Deprecation of `rake notes`, use `rails notes` instead
* Deprecation of `rails notes:custom ANNOTATION=custom`, `rails notes:optimize`, `rails notes:todo`, and `rails notes:fixme` in favor of passing `-annotations` or `-a` to `rails notes`
* They have all  been deprecrated in rails#33220
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.