[Fix #260] Fix target of `Rails/ContentTag` #526

tabuchik · 2021-08-15T17:32:28Z

This PR fixes target of Rails/ContentTag cop.
The reason is that it is unclear wheather content_tag is deprecated, and auto-correction may cause performance regression.

Following test cases are added.

If first argument is kebab case, it can be autocorrected to snake case method.
If first argument is snake case, it cannot be autocorrected.

#!/usr/bin/env rails runner

helper = ApplicationController.helpers
puts helper.tag("foo-bar")
# => <foo-bar />
puts helper.tag("foo_bar")
# => <foo_bar />
puts helper.tag.foo_bar
# => <foo-bar></foo-bar>

puts helper.tag("-foo")
# => <-foo />
puts helper.tag("_foo")
# => <_foo />
puts helper.tag._foo
# => <-foo></-foo>

Before submitting the PR make sure the following are checked:

Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.
Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.
If this is a new cop, consider making a corresponding update to the Rails Style Guide.

koic · 2021-08-17T01:07:34Z

config/default.yml

@@ -181,8 +181,9 @@ Rails/BulkChangeTable:
    - db/migrate/*.rb

 Rails/ContentTag:
-  Description: 'Use `tag` instead of `content_tag`.'
+  Description: 'Use `tag.something(` instead of `tag(:something`.'


IMHO, it's strange to write only the opening parenthesis.

Suggested change

Description: 'Use `tag.something(` instead of `tag(:something`.'

Description: 'Use `tag.something` instead of `tag(:something)`.'

koic · 2021-08-17T01:07:42Z

lib/rubocop/cop/rails/content_tag.rb

-      # NOTE: Allow `content_tag` when the first argument is a variable because
-      #      `content_tag(name)` is simpler rather than `tag.public_send(name)`.
+      # NOTE: Allow `tag` when the first argument is a variable because
+      #      `tag(name)` is simpler rather than `tag.public_send(name)`.


Can you add the following mention?

Suggested change

# `tag(name)` is simpler rather than `tag.public_send(name)`.

# `tag(name)` is simpler rather than `tag.public_send(name)`.

# And this cop will be renamed to something like `LegacyTag` in the future. (e.g. RuboCop Rails 2.0)

koic · 2021-08-17T01:07:44Z

lib/rubocop/cop/rails/content_tag.rb

-      #  content_tag(:p, 'Hello world!')
-      #  content_tag(:br)
+      #  tag(:p)
+      #  tag(:br, {class: 'classname'})


Can you omit the redundant parentheses?

Suggested change

# tag(:br, {class: 'classname'})

# tag(:br, class: 'classname')

koic · 2021-08-17T01:07:48Z

lib/rubocop/cop/rails/content_tag.rb

-      #  content_tag(name, 'Hello world!')
+      #  tag.p
+      #  tag.br(class: 'classname')
+      #  tag(name, {class: 'classname'})


Suggested change

# tag(name, {class: 'classname'})

# tag(name, class: 'classname')

koic · 2021-08-17T01:07:50Z

lib/rubocop/cop/rails/content_tag.rb

      class ContentTag < Base
        include RangeHelp
        extend AutoCorrector
        extend TargetRailsVersion

        minimum_target_rails_version 5.1

-        MSG = 'Use `tag` instead of `content_tag`.'
-        RESTRICT_ON_SEND = %i[content_tag].freeze
+        MSG = 'Use `tag.something(` instead of `tag(:something`.'


Suggested change

MSG = 'Use `tag.something(` instead of `tag(:something`.'

MSG = 'Use `tag.something` instead of `tag(:something)`.'

koic · 2021-08-17T01:13:40Z

Can you add the changelog entry and add VersionChanged: '2.12' to Rails/ContentTag configuration in the config/default.yml

andyw8 · 2021-08-17T01:38:35Z

spec/rubocop/cop/rails/content_tag_spec.rb

      RUBY
    end

-    it 'does not register an offense when `content_tag` is called with no arguments' do
+    it 'does not register an offense when first argument is string starts with underscore' do


Suggested change

it 'does not register an offense when first argument is string starts with underscore' do

it 'does not register an offense when first argument is a string which starts with an underscore' do

tabuchik · 2021-08-18T04:18:55Z

@koic @andyw8
I updated this pull request with your comment.
Thank you for reviews!

CHANGELOG.md

config/default.yml

tabuchik · 2021-08-18T04:40:03Z

@koic
Thanks for your quick response!
I accepted your suggestions and fix CI.

koic · 2021-08-18T05:00:40Z

Nice! Can you squash your commits into one?

Tweak for review comments Add a changelog entry Modify VersionChanged instead of VersionAdded Co-authored-by: Koichi ITO <[email protected]> Fix CHANGELOG.md Co-authored-by: Koichi ITO <[email protected]> Generate doc

tabuchik · 2021-08-18T05:59:41Z

@koic
Thank you! done

koic · 2021-08-19T16:08:36Z

Thanks!

Follow up to #526.

Follow up #526

The cop is buggy before rubocop-rails 2.12.0 [1][2], but to get that we'd need a newer theforeman-rubocop, which generates more offenses than I am willing to fix today. Disable the cop like it was disabled in foreman_ansible too [3]. [1] rubocop/rubocop-rails#260 [2] rubocop/rubocop-rails#526 [3] theforeman/foreman_ansible#357

The cop is buggy before rubocop-rails 2.12.0 [1][2], but to get that we'd need a newer theforeman-rubocop, which generates more offenses than I am willing to fix today. Disable the cop like it was disabled in foreman_ansible too [3]. [1] rubocop/rubocop-rails#260 [2] rubocop/rubocop-rails#526 [3] theforeman/foreman_ansible#357 (cherry picked from commit 98c9fee)

koic reviewed Aug 17, 2021

View reviewed changes

andyw8 reviewed Aug 17, 2021

View reviewed changes

koic reviewed Aug 18, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

koic reviewed Aug 18, 2021

View reviewed changes

config/default.yml Outdated Show resolved Hide resolved

[Fix #260] Fix target of Rails/ContentTag

3474d16

Tweak for review comments Add a changelog entry Modify VersionChanged instead of VersionAdded Co-authored-by: Koichi ITO <[email protected]> Fix CHANGELOG.md Co-authored-by: Koichi ITO <[email protected]> Generate doc

koic merged commit 6163f9a into rubocop:master Aug 19, 2021

koic mentioned this pull request Aug 20, 2021

Rails/ContentTag cop auto-correct creates invalid Ruby code in Ruby 3.0 #511

Closed

koic added a commit that referenced this pull request Aug 21, 2021

Tweak offense message for Rails/ContentTag

efe0ec9

Follow up to #526.

koic mentioned this pull request Sep 6, 2021

Rails/ContentTag Unsafe auto correct deemed safe #534

Closed

koic added a commit that referenced this pull request Dec 17, 2023

[Fix #1218] Fix a typo

1fe6385

Follow up #526

evgeni mentioned this pull request Jan 23, 2024

Fixes #36998 - declare development dependency on theforeman-rubocop Katello/katello#10857

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #260] Fix target of `Rails/ContentTag` #526

[Fix #260] Fix target of `Rails/ContentTag` #526

tabuchik commented Aug 15, 2021 •

edited

Loading

koic Aug 17, 2021

koic Aug 17, 2021

koic Aug 17, 2021

koic Aug 17, 2021

koic Aug 17, 2021

koic commented Aug 17, 2021

andyw8 Aug 17, 2021

tabuchik commented Aug 18, 2021

tabuchik commented Aug 18, 2021

koic commented Aug 18, 2021

tabuchik commented Aug 18, 2021

koic commented Aug 19, 2021

	Description: 'Use `tag.something(` instead of `tag(:something`.'
	Description: 'Use `tag.something` instead of `tag(:something)`.'

	# `tag(name)` is simpler rather than `tag.public_send(name)`.
	# `tag(name)` is simpler rather than `tag.public_send(name)`.
	# And this cop will be renamed to something like `LegacyTag` in the future. (e.g. RuboCop Rails 2.0)

	# tag(:br, {class: 'classname'})
	# tag(:br, class: 'classname')

	# tag(name, {class: 'classname'})
	# tag(name, class: 'classname')

	MSG = 'Use `tag.something(` instead of `tag(:something`.'
	MSG = 'Use `tag.something` instead of `tag(:something)`.'

	it 'does not register an offense when first argument is string starts with underscore' do
	it 'does not register an offense when first argument is a string which starts with an underscore' do

[Fix #260] Fix target of Rails/ContentTag #526

[Fix #260] Fix target of Rails/ContentTag #526

Conversation

tabuchik commented Aug 15, 2021 • edited Loading

koic Aug 17, 2021

Choose a reason for hiding this comment

koic Aug 17, 2021

Choose a reason for hiding this comment

koic Aug 17, 2021

Choose a reason for hiding this comment

koic Aug 17, 2021

Choose a reason for hiding this comment

koic Aug 17, 2021

Choose a reason for hiding this comment

koic commented Aug 17, 2021

andyw8 Aug 17, 2021

Choose a reason for hiding this comment

tabuchik commented Aug 18, 2021

tabuchik commented Aug 18, 2021

koic commented Aug 18, 2021

tabuchik commented Aug 18, 2021

koic commented Aug 19, 2021

[Fix #260] Fix target of `Rails/ContentTag` #526

[Fix #260] Fix target of `Rails/ContentTag` #526

tabuchik commented Aug 15, 2021 •

edited

Loading