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

autocorrect removes negation #6860

Closed
tbhi opened this issue Mar 22, 2019 · 2 comments · Fixed by #6951
Closed

autocorrect removes negation #6860

tbhi opened this issue Mar 22, 2019 · 2 comments · Fixed by #6951
Labels

Comments

@tbhi
Copy link

tbhi commented Mar 22, 2019

With file containing:

def get_m3u8_lines_from(manifest_body)
  manifest_body.each_line.map(&:strip).reject(&:empty?).select{|line| (not line.start_with? '#') }
end

Running autocorrect:

$ rubocop -V
0.66.0 (using Parser 2.5.3.0, running on ruby 2.6.2 x86_64-darwin18)
$ rubocop -a ~/Downloads/manifest.rb
Inspecting 1 file
C

Offenses:

Downloads/manifest.rb:2:3: C: [Corrected] Style/InverseMethods: Use reject instead of inverting select.
  manifest_body.each_line.map(&:strip).reject(&:empty?).select{|line| (not line.start_with? '#') }
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Downloads/manifest.rb:2:3: C: [Corrected] Style/InverseMethods: Use select instead of inverting reject.
  manifest_body.each_line.map(&:strip).reject(&:empty?).reject {|line| (!line.start_with? '#') }
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Downloads/manifest.rb:2:63: C: [Corrected] Layout/SpaceBeforeBlockBraces: Space missing to the left of {.
  manifest_body.each_line.map(&:strip).reject(&:empty?).select{|line| (not line.start_with? '#') }
                                                              ^
Downloads/manifest.rb:2:63: C: [Corrected] Layout/SpaceInsideBlockBraces: Space between { and | missing.
  manifest_body.each_line.map(&:strip).reject(&:empty?).select{|line| (not line.start_with? '#') }
                                                              ^^
Downloads/manifest.rb:2:64: C: [Corrected] Layout/SpaceInsideBlockBraces: Space between { and | missing.
  manifest_body.each_line.map(&:strip).reject(&:empty?).reject {|line| (!line.start_with? '#') }
                                                               ^^
Downloads/manifest.rb:2:72: C: [Corrected] Style/Not: Use ! instead of not.
  manifest_body.each_line.map(&:strip).reject(&:empty?).select{|line| (not line.start_with? '#') }
                                                                       ^^^
Downloads/manifest.rb:2:81: C: Metrics/LineLength: Line is too long. [96/80]
  manifest_body.each_line.map(&:strip).reject(&:empty?).select { |line| (line.start_with? '#') }
                                                                                ^^^^^^^^^^^^^^^^

1 file inspected, 7 offenses detected, 6 offenses corrected

Ends up losing the negation in the output:

def get_m3u8_lines_from(manifest_body)
  manifest_body.each_line.map(&:strip).reject(&:empty?).select { |line| (line.start_with? '#') }
end
@koic koic added the bug label Mar 23, 2019
@dduugg
Copy link
Contributor

dduugg commented Apr 12, 2019

In case useful, a more minimal repro:

x.select {|y| not y.z}

is autocorrected to

x.select(&:z)

using rubocop defaults

@hoshinotsuyoshi
Copy link
Contributor

hoshinotsuyoshi commented Apr 20, 2019

The comment #6860 (comment) is so helpful to me. thank you! @dduugg

I believe that the problem is that Style/InverseMethods cop and Style/Not cop combination cannot auto-correct such code at once.

file:

x.select { |y| not y.z }

auto-correct:

$ bundle exec rubocop -a c.rb --only Style/InverseMethods,Style/Not
Inspecting 1 file
C

Offenses:

c.rb:1:1: C: [Corrected] Style/InverseMethods: Use reject instead of inverting select.
x.select { |y| not y.z }
^^^^^^^^^^^^^^^^^^^^^^^^
c.rb:1:1: C: [Corrected] Style/InverseMethods: Use select instead of inverting reject.
x.reject { |y| !y.z }
^^^^^^^^^^^^^^^^^^^^^
c.rb:1:16: C: [Corrected] Style/Not: Use ! instead of not.
x.select { |y| not y.z }
               ^^^

1 file inspected, 3 offenses detected, 3 offenses corrected

corrected badly:

x.select { |y| y.z }

hoshinotsuyoshi added a commit to hoshinotsuyoshi/rubocop that referenced this issue Apr 22, 2019
…hods` and `Style/Not`.

Prevent conflict problem like below:

file:
```
x.select { |y| not y.z }
```

auto-correct:
```
$ bundle exec rubocop -a c.rb --only Style/InverseMethods,Style/Not
Inspecting 1 file
C

Offenses:

c.rb:1:1: C: [Corrected] Style/InverseMethods: Use reject instead of inverting select.
x.select { |y| not y.z }
^^^^^^^^^^^^^^^^^^^^^^^^
c.rb:1:1: C: [Corrected] Style/InverseMethods: Use select instead of inverting reject.
x.reject { |y| !y.z }
^^^^^^^^^^^^^^^^^^^^^
c.rb:1:16: C: [Corrected] Style/Not: Use ! instead of not.
x.select { |y| not y.z }
               ^^^

1 file inspected, 3 offenses detected, 3 offenses corrected
```

corrected badly:
```
x.select { |y| y.z }
```
bbatsov pushed a commit that referenced this issue Apr 22, 2019
…nd `Style/Not`.

Prevent conflict problem like below:

file:
```
x.select { |y| not y.z }
```

auto-correct:
```
$ bundle exec rubocop -a c.rb --only Style/InverseMethods,Style/Not
Inspecting 1 file
C

Offenses:

c.rb:1:1: C: [Corrected] Style/InverseMethods: Use reject instead of inverting select.
x.select { |y| not y.z }
^^^^^^^^^^^^^^^^^^^^^^^^
c.rb:1:1: C: [Corrected] Style/InverseMethods: Use select instead of inverting reject.
x.reject { |y| !y.z }
^^^^^^^^^^^^^^^^^^^^^
c.rb:1:16: C: [Corrected] Style/Not: Use ! instead of not.
x.select { |y| not y.z }
               ^^^

1 file inspected, 3 offenses detected, 3 offenses corrected
```

corrected badly:
```
x.select { |y| y.z }
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants