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.

Sign up for GitHub

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

Jump to bottom

blank? expression is incorrectly converted to presence expression #116

Closed
servant714 opened this issue Aug 26, 2019 · 3 comments · Fixed by #117
Closed

blank? expression is incorrectly converted to presence expression #116

servant714 opened this issue Aug 26, 2019 · 3 comments · Fixed by #117
Labels
bug Something isn't working

Comments

@servant714
Copy link

The expression "x.blank? 1 : x" is incorrectly corrected to "x.presence".

Expected behavior

Expected the line "puts [1, nil, 0, 3].map { |x| x.blank? ? 1 : x }.sum" to be unchanged.

Actual behavior

The line "puts [1, nil, 0, 3].map { |x| x.blank? ? 1 : x }.sum"
was changed to "puts [1, nil, 0, 3].map(&:presence).sum"

Steps to reproduce the problem

In my rails directory, I create the file "bug.rb"
with the line "puts [1, nil, 0, 3].map { |x| x.blank? ? 1 : x }.sum".
Ran "rails runner bug.rb".
It outputs "5", which is the correct and expected value.
Ran "rubocop --auto-correct", which changes the file bug.rb.
Ran "rails runner bug.rb".
It outputs a Traceback since the nil value was not converted to "1" like the original expression.

RuboCop version

bundle exec rubocop -V
0.74.0 (using Parser 2.6.3.0, running on ruby 2.6.3 x86_64-linux)

From my Gemfile.lock,
rubocop-performance is 1.4.1
rubocop-rails is 2.3.1


My .rubocop.yml has the lines
require:
  - rubocop-performance
  - rubocop-rails
AllCops:
  TargetRubyVersion: 2.6
Style/FrozenStringLiteralComment:
  Enabled: false
Style/AutoResourceCleanup:
  Enabled: true
Style/ImplicitRuntimeError:
  Enabled: true
Rails:
  Enabled: true
Rails/SaveBang:
  Enabled: true
@servant714
Copy link
Author

I changed my code to use the expression "x.present? ? x : 1" hoping it would not auto-correct, but it changes that expression to "x.presence", which is incorrect.

@Drenmi Drenmi added the bug Something isn't working label Aug 27, 2019
koic added a commit to koic/rubocop-rails that referenced this issue Aug 27, 2019
@koic
[rubocop#116] Fix an incorrect autocorrect for Rails/Prensence
4e875be 
Fixes rubocop#116 and adds regression test for rubocop#115.

This PR fixes an incorrect autocorrect for `Rails/Presence` when
`else` branch of ternary operator is not nil.

The following is a reproduction procedure.

```ruby
# example.rb
a.blank? ? 1 : a
```

```console
% bundle exec rubocop --only Rails/Presence -a
Inspecting 1 file
C

Offenses:

example.rb:1:1: C: [Corrected] Rails/Presence: Use a.presence instead of
a.blank? ? 1 : a.
a.blank? ? 1 : a
^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected
```

```diff
-a.blank? ? 1 : a
+a.presence
```

This PR is auto-corrected to `a.presence || 1`.
@koic koic mentioned this issue Aug 27, 2019
Merged
8 tasks
koic added a commit to koic/rubocop-rails that referenced this issue Aug 27, 2019
@koic
[rubocop#116] Fix an incorrect autocorrect for Rails/Prensence
b32dc36 
Fixes rubocop#116 and adds regression tests for rubocop#115.

This PR fixes an incorrect autocorrect for `Rails/Presence` when
`else` branch of ternary operator is not nil.

The following is a reproduction procedure.

```ruby
# example.rb
a.blank? ? 1 : a
```

```console
% bundle exec rubocop --only Rails/Presence -a
Inspecting 1 file
C

Offenses:

example.rb:1:1: C: [Corrected] Rails/Presence: Use a.presence instead of
a.blank? ? 1 : a.
a.blank? ? 1 : a
^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected
```

```diff
-a.blank? ? 1 : a
+a.presence
```

This PR is auto-corrected to `a.presence || 1`.
koic added a commit to koic/rubocop-rails that referenced this issue Aug 27, 2019
@koic
[Fix rubocop#116] Fix an incorrect autocorrect for Rails/Prensence
a0f162b 
Fixes rubocop#116 and adds regression tests for rubocop#115.

This PR fixes an incorrect autocorrect for `Rails/Presence` when
`else` branch of ternary operator is not nil.

The following is a reproduction procedure.

```ruby
# example.rb
a.blank? ? 1 : a
```

```console
% bundle exec rubocop --only Rails/Presence -a
Inspecting 1 file
C

Offenses:

example.rb:1:1: C: [Corrected] Rails/Presence: Use a.presence instead of
a.blank? ? 1 : a.
a.blank? ? 1 : a
^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected
```

```diff
-a.blank? ? 1 : a
+a.presence
```

This PR will auto-corrected to `a.presence || 1`.
koic added a commit to koic/rubocop-rails that referenced this issue Aug 27, 2019
@koic
[Fix rubocop#116] Fix an incorrect autocorrect for Rails/Prensence
27008a8 
Fixes rubocop#116 and adds regression tests for rubocop#115.

This PR fixes an incorrect autocorrect for `Rails/Presence` when
`else` branch of ternary operator is not nil.

The following is a reproduction procedure.

```ruby
# example.rb
a.blank? ? 1 : a
```

```console
% bundle exec rubocop --only Rails/Presence -a
Inspecting 1 file
C

Offenses:

example.rb:1:1: C: [Corrected] Rails/Presence: Use a.presence instead of
a.blank? ? 1 : a.
a.blank? ? 1 : a
^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected
```

```diff
-a.blank? ? 1 : a
+a.presence
```

This PR will auto-correct it to `a.presence || 1`.
@jhirn
Copy link

jhirn commented Aug 27, 2019

Another example

Using rubocop-rails (2.3.1)

p[2].present? ? p[2] : p[1]  =>  p[2].presence || p(1)

koic added a commit to koic/rubocop-rails that referenced this issue Aug 31, 2019
@koic
[Fix rubocop#116] Fix an incorrect autocorrect for Rails/Prensence
3a0998c 
Fixes rubocop#116 and adds regression tests for rubocop#115.

This PR fixes an incorrect autocorrect for `Rails/Presence` when
`else` branch of ternary operator is not nil.

The following is a reproduction procedure.

```ruby
# example.rb
a.blank? ? 1 : a
```

```console
% bundle exec rubocop --only Rails/Presence -a
Inspecting 1 file
C

Offenses:

example.rb:1:1: C: [Corrected] Rails/Presence: Use a.presence instead of
a.blank? ? 1 : a.
a.blank? ? 1 : a
^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected
```

```diff
-a.blank? ? 1 : a
+a.presence
```

This PR will auto-correct it to `a.presence || 1`.
@koic koic closed this as completed in #117 Sep 1, 2019
koic added a commit that referenced this issue Sep 1, 2019
@koic
Merge pull request #117 from koic/fix_an_incorrect_autocorrect_for_ra…
ba618ca 
…ils_presence

[Fix #116] Fix an incorrect autocorrect for `Rails/Prensence`
@koic
Copy link
Member

koic commented Sep 1, 2019

RuboCop Rails 2.3.2 has been released to resolve this issue. Thank you.

@touhouota touhouota mentioned this issue Sep 2, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Fix #116] Fix an incorrect autocorrect for Rails/Prensence koic/rubocop-rails
4 participants
@koic @jhirn @Drenmi @servant714