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

Add new Rails/ContentTag cop #242

Merged
merged 1 commit into from
May 4, 2020
Merged

Add new Rails/ContentTag cop #242

merged 1 commit into from
May 4, 2020

Conversation

tabuchik
Copy link
Contributor

@tabuchik tabuchik commented May 2, 2020

This PR introduces a cop to check that tag is used instead of content_tag, because content_tag is legacy syntax.

Note: this is legacy syntax, see tag method description for details.
https://api.rubyonrails.org/classes/ActionView/Helpers/TagHelper.html#method-i-content_tag


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 to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • 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.

config/default.yml Outdated Show resolved Hide resolved
config/default.yml Outdated Show resolved Hide resolved
@koic
Copy link
Member

koic commented May 3, 2020

Can you add a test case for using block? e.g:

content_tag(:div) { content_tag(:strong, 'Hi') }

@tabuchik tabuchik changed the title Add new Rails/TagInsteadOfContentTag cop Add new Rails/ContentTag cop May 3, 2020
config/default.yml Outdated Show resolved Hide resolved
config/default.yml Outdated Show resolved Hide resolved
@tabuchik
Copy link
Contributor Author

tabuchik commented May 4, 2020

@koic
Thank you for the frequent reviews!!
I fixed all of them.

- 'https://github.com/rails/rails/issues/25195'
- 'https://api.rubyonrails.org/classes/ActionView/Helpers/TagHelper.html#method-i-content_tag'
Enabled: true
Safe: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, can you omit Safe: true because it is safe by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see.

@koic
Copy link
Member

koic commented May 4, 2020

This looks good to me. Can you squash your commits into one?

@tabuchik
Copy link
Contributor Author

tabuchik commented May 4, 2020

Remove unnecessary Safe: true, and all the commits are squashed.
Thank you for your reviews!

@koic koic merged commit 891c6aa into rubocop:master May 4, 2020
@koic
Copy link
Member

koic commented May 4, 2020

Thank you!

@rmacklin
Copy link

@koic Would you be open to publishing a new release that includes this change (and the other changes since https://github.com/rubocop-hq/rubocop-rails/releases/tag/v2.5.2)?

@koic
Copy link
Member

koic commented May 12, 2020

I've found several issues with this cop and I plan to release 2.6 that includes this one after fixing them.

@rmacklin
Copy link

Gotcha, thank you for the update (and the speedy reply)!

koic added a commit to koic/rubocop-rails that referenced this pull request May 14, 2020
Follow up to rubocop#242.

`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`.
koic added a commit to koic/rubocop-rails that referenced this pull request May 16, 2020
Follow up to rubocop#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`.
koic added a commit to koic/rubocop-rails that referenced this pull request May 16, 2020
Follow up to rubocop#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`.
koic added a commit to koic/rubocop-rails that referenced this pull request May 16, 2020
Follow up to rubocop#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`.
koic added a commit to koic/rubocop-rails that referenced this pull request May 16, 2020
Follow up to rubocop#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`.
koic added a commit to koic/rubocop-rails that referenced this pull request May 16, 2020
Follow up to rubocop#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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants