From 6ce8dd17ba06f6a0f7794083c89549fb9e7d75cc Mon Sep 17 00:00:00 2001 From: tabuchi0919 Date: Mon, 4 May 2020 19:22:38 +0900 Subject: [PATCH] Add `Rails/ContentTag` cop --- CHANGELOG.md | 2 + config/default.yml | 8 ++ lib/rubocop/cop/rails/content_tag.rb | 73 +++++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + manual/cops.md | 1 + manual/cops_rails.md | 26 +++++ spec/rubocop/cop/rails/content_tag_spec.rb | 118 +++++++++++++++++++++ 7 files changed, 229 insertions(+) create mode 100644 lib/rubocop/cop/rails/content_tag.rb create mode 100644 spec/rubocop/cop/rails/content_tag_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 8cd37370d5..ad81d33066 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * [#51](https://github.com/rubocop-hq/rubocop-rails/issues/51): Add allowed receiver class names option for `Rails/DynamicFindBy`. ([@tejasbubane][]) * [#211](https://github.com/rubocop-hq/rubocop-rails/issues/211): Add autocorrect to `Rails/RakeEnvironment` cop. ([@tejasbubane][]) +* [#242](https://github.com/rubocop-hq/rubocop-rails/pull/242): Add `Rails/ContentTag` cop. ([@tabuchi0919][]) ### Bug fixes @@ -188,3 +189,4 @@ [@hoshinotsuyoshi]: https://github.com/hoshinotsuyoshi [@tejasbubane]: https://github.com/tejasbubane [@diogoosorio]: https://github.com/diogoosorio +[@tabuchi0919]: https://github.com/tabuchi0919 diff --git a/config/default.yml b/config/default.yml index d7cc7dbf8b..5a38a7e9de 100644 --- a/config/default.yml +++ b/config/default.yml @@ -119,6 +119,14 @@ Rails/BulkChangeTable: Include: - db/migrate/*.rb +Rails/ContentTag: + Description: 'Use `tag` instead of `content_tag`.' + Reference: + - 'https://github.com/rails/rails/issues/25195' + - 'https://api.rubyonrails.org/classes/ActionView/Helpers/TagHelper.html#method-i-content_tag' + Enabled: true + VersionAdded: '2.6' + Rails/CreateTableWithTimestamps: Description: >- Checks the migration for which timestamps are not included diff --git a/lib/rubocop/cop/rails/content_tag.rb b/lib/rubocop/cop/rails/content_tag.rb new file mode 100644 index 0000000000..3ba3a50176 --- /dev/null +++ b/lib/rubocop/cop/rails/content_tag.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # This cop checks that `tag` is used instead of `content_tag` + # because `content_tag` is legacy syntax. + # + # @example + # # bad + # content_tag(:p, 'Hello world!') + # content_tag(:br) + # + # # good + # tag.p('Hello world!') + # tag.br + class ContentTag < Cop + include RangeHelp + extend TargetRailsVersion + + minimum_target_rails_version 5.1 + + MSG = 'Use `tag` instead of `content_tag`.' + + def on_send(node) + return unless node.method?(:content_tag) + + add_offense(node) + end + + def autocorrect(node) + lambda do |corrector| + if method_name?(node.first_argument) + replace_method_with_tag_method(corrector, node) + remove_first_argument(corrector, node) + else + corrector.replace(node.loc.selector, 'tag') + end + end + end + + private + + def method_name?(node) + return false unless node.str_type? || node.sym_type? + + /^[a-zA-Z_][a-zA-Z_0-9]*$/.match?(node.value) + end + + def replace_method_with_tag_method(corrector, node) + corrector.replace( + node.loc.selector, + "tag.#{node.first_argument.value}" + ) + end + + def remove_first_argument(corrector, node) + if node.arguments.length > 1 + corrector.remove( + range_between(child_node_beg(node, 0), child_node_beg(node, 1)) + ) + elsif node.arguments.length == 1 + corrector.remove(node.arguments[0].loc.expression) + end + end + + def child_node_beg(node, index) + node.arguments[index].loc.expression.begin_pos + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 58404c8e83..947c398637 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -16,6 +16,7 @@ require_relative 'rails/belongs_to' require_relative 'rails/blank' require_relative 'rails/bulk_change_table' +require_relative 'rails/content_tag' require_relative 'rails/create_table_with_timestamps' require_relative 'rails/date' require_relative 'rails/delegate' diff --git a/manual/cops.md b/manual/cops.md index 7d1487530d..e83635709c 100644 --- a/manual/cops.md +++ b/manual/cops.md @@ -13,6 +13,7 @@ * [Rails/BelongsTo](cops_rails.md#railsbelongsto) * [Rails/Blank](cops_rails.md#railsblank) * [Rails/BulkChangeTable](cops_rails.md#railsbulkchangetable) +* [Rails/ContentTag](cops_rails.md#railscontenttag) * [Rails/CreateTableWithTimestamps](cops_rails.md#railscreatetablewithtimestamps) * [Rails/Date](cops_rails.md#railsdate) * [Rails/Delegate](cops_rails.md#railsdelegate) diff --git a/manual/cops_rails.md b/manual/cops_rails.md index f0ff5f2fb9..c0316e69d7 100644 --- a/manual/cops_rails.md +++ b/manual/cops_rails.md @@ -443,6 +443,32 @@ Name | Default value | Configurable values Database | `` | `mysql`, `postgresql` Include | `db/migrate/*.rb` | Array +## Rails/ContentTag + +Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged +--- | --- | --- | --- | --- +Enabled | Yes | Yes | 2.6 | - + +This cop checks that `tag` is used instead of `content_tag` +because `content_tag` is legacy syntax. + +### Examples + +```ruby +# bad +content_tag(:p, 'Hello world!') +content_tag(:br) + +# good +tag.p('Hello world!') +tag.br +``` + +### References + +* [https://github.com/rails/rails/issues/25195](https://github.com/rails/rails/issues/25195) +* [https://api.rubyonrails.org/classes/ActionView/Helpers/TagHelper.html#method-i-content_tag](https://api.rubyonrails.org/classes/ActionView/Helpers/TagHelper.html#method-i-content_tag) + ## Rails/CreateTableWithTimestamps Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged diff --git a/spec/rubocop/cop/rails/content_tag_spec.rb b/spec/rubocop/cop/rails/content_tag_spec.rb new file mode 100644 index 0000000000..f2be1b8f47 --- /dev/null +++ b/spec/rubocop/cop/rails/content_tag_spec.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::ContentTag, :config do + subject(:cop) { described_class.new(config) } + + context 'Rails 5.0', :rails50 do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + content_tag(:p, 'Hello world!') + RUBY + end + + it 'does not register an offense with empty tag' do + expect_no_offenses(<<~RUBY) + content_tag(:br) + RUBY + end + + it 'does not register an offense with array of classnames' do + expect_no_offenses(<<~RUBY) + content_tag(:div, "Hello world!", class: ["strong", "highlight"]) + RUBY + end + + it 'does not register an offense with nested content_tag' do + expect_no_offenses(<<~RUBY) + content_tag(:div) { content_tag(:strong, 'Hi') } + RUBY + end + end + + context 'Rails 5.1', :rails51 do + it 'corrects an offence' do + expect_offense(<<~RUBY) + content_tag(:p, 'Hello world!') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `tag` instead of `content_tag`. + RUBY + expect_correction(<<~RUBY) + tag.p('Hello world!') + RUBY + end + + it 'corrects an offence with empty tag' do + expect_offense(<<~RUBY) + content_tag(:br) + ^^^^^^^^^^^^^^^^ Use `tag` instead of `content_tag`. + RUBY + expect_correction(<<~RUBY) + tag.br() + RUBY + end + + it 'corrects an offence with array of classnames' do + expect_offense(<<~RUBY) + content_tag(:div, "Hello world!", class: ["strong", "highlight"]) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `tag` instead of `content_tag`. + RUBY + expect_correction(<<~RUBY) + tag.div("Hello world!", class: ["strong", "highlight"]) + RUBY + end + + it 'corrects an offence with nested content_tag' do + expect_offense(<<~RUBY) + content_tag(:div) { content_tag(:strong, 'Hi') } + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `tag` instead of `content_tag`. + ^^^^^^^^^^^^^^^^^ Use `tag` instead of `content_tag`. + RUBY + expect_correction(<<~RUBY) + tag.div() { tag.strong('Hi') } + RUBY + end + + it 'corrects an offence when first argument is hash' do + expect_offense(<<~RUBY) + content_tag({foo: 1}) + ^^^^^^^^^^^^^^^^^^^^^ Use `tag` instead of `content_tag`. + RUBY + expect_correction(<<~RUBY) + tag({foo: 1}) + RUBY + end + + it 'corrects an offence when first argument is non-identifier string' do + expect_offense(<<~RUBY) + content_tag('foo-bar') + ^^^^^^^^^^^^^^^^^^^^^^ Use `tag` instead of `content_tag`. + RUBY + expect_correction(<<~RUBY) + tag('foo-bar') + RUBY + end + + it 'does not register an offence when `tag` is used with an argument' do + expect_no_offenses(<<~RUBY) + tag.p('Hello world!') + RUBY + end + + it 'does not register an offence when `tag` is used without arguments' do + expect_no_offenses(<<~RUBY) + tag.br + RUBY + end + + it 'does not register an offence when `tag` is used with arguments' do + expect_no_offenses(<<~RUBY) + tag.div("Hello world!", class: ["strong", "highlight"]) + RUBY + end + + it 'does not register an offence when `tag` is nested' do + expect_no_offenses(<<~RUBY) + tag.div() { tag.strong('Hi') } + RUBY + end + end +end