From 43a721c67d2db10b96ed1405250b63a01772d754 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Thu, 7 May 2020 20:02:10 +0900 Subject: [PATCH] Fix an incorrect autocorrect for `Rails/ContentTag` cop Follow up to #242. This PR allows `content_tag` when the first argument is a variable because `content_tag(name)` is simpler rather than `tag.public_send(name)`. When `public_send` is used, it becomes complicated as follows. First, `content_tag(name)` cannot be corrected to `tag.name` when the first argument is a variable. So use `public_send` and prevent the following errors in that case: ## Case 1: Using `public_send` to prevent the following `NoMethodError` Original code: ```ruby content_tag(name, 'foo', class: 'bar') ``` Auto-corrected code (before): ```ruby tag(name, 'foo', class: 'bar') #=> NoMethodError (undefined method `each_pair' for "foo":String) ``` Auto-corrected code (after): ```ruby tag.public_send(name, 'foo', class: 'bar') ``` ## Case 2: Using `symbolize_keys` to prevent the following `ArgumentError` Original code: ```ruby content_tag(name, 'foo', {'class' => 'bar'}) ``` Auto-corrected code (before): ```ruby tag.public_send(name, 'foo', {'class' => 'bar'}) #=> `ArgumentError (wrong number of arguments (given 3, expected 1..2))` ``` Auto-corrected code (after): ```ruby tag.public_send(name, 'foo', {'class' => 'bar'}.symbolize_keys) ``` The `symbolize_keys` may not be needed, but for safe auto-correction it will be added if optional argument keys are not all symbols. ## Case 3: Using `o ? o.symbolize_keys : {}` to prevent the following `ArgumentError` Original code: ```ruby content_tag(name, 'foo', options) ``` Auto-corrected code (before): When the third argument is `nil`. ```ruby options = nil tag.public_send(name, 'foo', options) #=> `ArgumentError (wrong number of arguments (given 3, expected 1..2))` ``` Auto-corrected code (after): ```ruby tag.public_send(name, 'foo', options ? options.symbolize_keys : {}) ``` Guard with the empty hash in case the third argument is `nil`. --- lib/rubocop/cop/rails/content_tag.rb | 9 +++ manual/cops_rails.md | 6 ++ spec/rubocop/cop/rails/content_tag_spec.rb | 75 ++++++++++++++++++++++ 3 files changed, 90 insertions(+) diff --git a/lib/rubocop/cop/rails/content_tag.rb b/lib/rubocop/cop/rails/content_tag.rb index 3ba3a50176..d9320849e8 100644 --- a/lib/rubocop/cop/rails/content_tag.rb +++ b/lib/rubocop/cop/rails/content_tag.rb @@ -6,6 +6,11 @@ module Rails # This cop checks that `tag` is used instead of `content_tag` # because `content_tag` is legacy syntax. # + # NOTE: + # + # Allow `content_tag` when the first argument is a variable because + # `content_tag(name)` is simpler rather than `tag.public_send(name)`. + # # @example # # bad # content_tag(:p, 'Hello world!') @@ -14,6 +19,7 @@ module Rails # # good # tag.p('Hello world!') # tag.br + # content_tag(name, 'Hello world!') class ContentTag < Cop include RangeHelp extend TargetRailsVersion @@ -25,6 +31,9 @@ class ContentTag < Cop def on_send(node) return unless node.method?(:content_tag) + first_argument = node.first_argument + return if first_argument.variable? || first_argument.send_type? || first_argument.const_type? + add_offense(node) end diff --git a/manual/cops_rails.md b/manual/cops_rails.md index c0316e69d7..392eadf80f 100644 --- a/manual/cops_rails.md +++ b/manual/cops_rails.md @@ -452,6 +452,11 @@ Enabled | Yes | Yes | 2.6 | - This cop checks that `tag` is used instead of `content_tag` because `content_tag` is legacy syntax. +NOTE: + + Allow `content_tag` when the first argument is a variable because + `content_tag(name)` is simpler rather than `tag.public_send(name)`. + ### Examples ```ruby @@ -462,6 +467,7 @@ content_tag(:br) # good tag.p('Hello world!') tag.br +content_tag(name, 'Hello world!') ``` ### References diff --git a/spec/rubocop/cop/rails/content_tag_spec.rb b/spec/rubocop/cop/rails/content_tag_spec.rb index f2be1b8f47..699cde51e0 100644 --- a/spec/rubocop/cop/rails/content_tag_spec.rb +++ b/spec/rubocop/cop/rails/content_tag_spec.rb @@ -60,6 +60,81 @@ RUBY end + # Using `public_send` to prevent the following `NoMethodError`. + # + # tag(name, 'foo', class: 'bar') + # + # #=> NoMethodError (undefined method `each_pair' for "foo":String) + context 'when the first argument is a method or a variable' do + it 'does not register an offence when the first argument is a method' do + expect_no_offenses(<<~RUBY) + content_tag(name, "Hello world!", class: ["strong", "highlight"]) + RUBY + end + + it 'does not register an offence when the first argument is a lvar' do + expect_no_offenses(<<~RUBY) + name = do_something + content_tag(name, "Hello world!", class: ["strong", "highlight"]) + RUBY + end + + it 'does not register an offence when the first argument is an ivar' do + expect_no_offenses(<<~RUBY) + content_tag(@name, "Hello world!", class: ["strong", "highlight"]) + RUBY + end + + it 'does not register an offence when the first argument is a cvar' do + expect_no_offenses(<<~RUBY) + content_tag(@@name, "Hello world!", class: ["strong", "highlight"]) + RUBY + end + + it 'does not register an offence when the first argument is a gvar' do + expect_no_offenses(<<~RUBY) + content_tag($name, "Hello world!", class: ["strong", "highlight"]) + RUBY + end + + it 'does not register an offence when the first argument is a constant' do + expect_no_offenses(<<~RUBY) + content_tag(CONST, "Hello world!", class: ["strong", "highlight"]) + RUBY + end + + # Using `symbolize_keys` to prevent the following `ArgumentError`. + # + # tag.public_send(name, 'foo', {'class' => 'bar'}) + # + # `ArgumentError (wrong number of arguments (given 3, expected 1..2))` + context 'when options argument is a hash or a variable' do + it 'does not register an offence when the last argument is a variable' do + expect_no_offenses(<<~RUBY) + content_tag(name, "Hello world!", options) + RUBY + end + + it 'does not register an offence when the last argument is a hash of string keys with brace' do + expect_no_offenses(<<~RUBY) + content_tag(name, "Hello world!", {'class' => 'foo'}) + RUBY + end + + it 'does not register an offence when the last argument is a hash of string keys without brace' do + expect_no_offenses(<<~RUBY) + content_tag(name, "Hello world!", 'class' => 'foo') + RUBY + end + + it 'does not register an offence when the last argument is a hash of string and symbol keys without brace' do + expect_no_offenses(<<~RUBY) + content_tag(name, "Hello world!", 'class' => 'foo', id: 'baz') + RUBY + end + end + end + it 'corrects an offence with nested content_tag' do expect_offense(<<~RUBY) content_tag(:div) { content_tag(:strong, 'Hi') }