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

Fix an incorrect autocorrect for Rails/ContentTag cop #244

Merged

Conversation

koic
Copy link
Member

@koic koic commented May 14, 2020

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:

content_tag(name, 'foo', class: 'bar')

Auto-corrected code (before):

tag(name, 'foo', class: 'bar')
#=> NoMethodError (undefined method `each_pair' for "foo":String)

Auto-corrected code (after):

tag.public_send(name, 'foo', class: 'bar')

Case 2: Using symbolize_keys to prevent the following ArgumentError

Original code:

content_tag(name, 'foo', {'class' => 'bar'})

Auto-corrected code (before):

tag.public_send(name, 'foo', {'class' => 'bar'})
#=> `ArgumentError (wrong number of arguments (given 3, expected 1..2))`

Auto-corrected code (after):

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:

content_tag(name, 'foo', options)

Auto-corrected code (before):

When the third argument is nil.

options = nil
tag.public_send(name, 'foo', options)
#=> `ArgumentError (wrong number of arguments (given 3, expected 1..2))`

Auto-corrected code (after):

tag.public_send(name, 'foo', options ? options.symbolize_keys : {})

Guard with the empty hash in case the third argument is nil.


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.

@koic
Copy link
Member Author

koic commented May 14, 2020

This is a draft PR. I'm investigating if there is a better code to auto-correct it.

@koic
Copy link
Member Author

koic commented May 14, 2020

/cc @tabuchi0919

@koic
Copy link
Member Author

koic commented May 14, 2020

This is the note that has just been decided. It takes allowing content_tag rather than using public_send when the first argument is a variable. Thank you for advising @amatsuda.

@koic koic force-pushed the fix_an_incorrect_autocorrect_for_content_tag branch 4 times, most recently from 8a283a7 to e5fd936 Compare May 16, 2020 07:17
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 koic force-pushed the fix_an_incorrect_autocorrect_for_content_tag branch from e5fd936 to 9b921bd Compare May 16, 2020 07:22
@koic
Copy link
Member Author

koic commented May 16, 2020

I updated this PR to allow content_tag when the first argument is a variable, method, or constant because content_tag(name) is simpler rather than tag.public_send(name).

@koic koic merged commit bfb60a0 into rubocop:master May 18, 2020
@koic koic deleted the fix_an_incorrect_autocorrect_for_content_tag branch May 18, 2020 23:49
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.

1 participant