Adding “prefix” and “postfix” styles to Style/NegatedIf #4104

brandonweiss · 2017-03-09T19:37:44Z

[Fixes #3961]

Some people have a strong preference against using unless with an if
statement in multi-line/block form:

unless foo
  bar
end

But they think it still makes sense to use it in single-line/modifier
form:

bar unless foo

I’ve added three supported styles:

prefix will only enforce using unless in the multi-line/block form
postfix will only enforce using unless in the
single-line/modifier form
both will enforce using unless in both the prefix and postfix
forms

brandonweiss · 2017-03-09T19:43:28Z

@bbatsov This is my first-time contributing to RuboCop and working with ASTs, so please let me know if I didn't do something right!

One thing I wasn't sure about was how to handle the both style? Should I add a specific check for it rather than falling through if it's not prefix or postfix? Or is this OK because RuboCop will complain if you try to set something other than the supported styles?

brandonweiss · 2017-03-09T19:50:46Z

Also, I guess I have to add the checks for the different styles to the autocorrect method as well?

brandonweiss · 2017-03-10T02:42:11Z

@Drenmi That's weird, I wrote some tests for autocorrect that I expected to fail and they all passed. It seems to magically do the right thing without modifying the autocorrect block… what am I missing?

brandonweiss · 2017-03-10T02:46:07Z

@Drenmi I pushed them up if you want to take a look…

bbatsov · 2017-03-11T07:14:35Z

lib/rubocop/cop/style/negated_if.rb

-      # without else are considered.
+      # without else are considered. There are three different styles:
+      #
+      # both - enforces `unless` for `prefix` and `postfix` conditionals


You should add # good and # bad and @example annotations to the code examples.

👍 How's that? There seems to be some inconsistency in the formatting between cops—I went with what I think looks good, but let me know which cop I should mimic if this isn't right.

bbatsov · 2017-03-11T07:15:30Z

config/default.yml

@@ -861,6 +861,13 @@ Style/MultilineOperationIndentation:
  # But it can be overridden by setting this parameter
  IndentationWidth: ~

+Style/NegatedIf:
+  EnforcedStyle: both
+  SupportedStyles:


I'd add some descriptions of the styles here.

👍 Is this enough? There seems to be some inconsistency in how much of a description to give—some almost duplicate the documentation in the cop itself…

bbatsov · 2017-03-11T07:17:44Z

spec/rubocop/cop/style/negated_if_spec.rb

+      expect(cop.offenses).to be_empty
+    end
+
+    it 'autocorrects by replacing if not with unless' do


None of those examples seem specific to the both style.

I'm not sure what you mean. Do you mean the three autocorrect tests at the end? Or all of the tests inside of the “both” block? Or do you mean it doesn't seem to be testing the “both” style because we're not specifically setting the EnforcedStyle configuration to “both”?

What I did was I moved all the existing tests into the “both” block, and then added new tests specifically for “prefix” and “postfix”. My thinking was since “prefix” and “postfix” styles didn't previously exist, all the existing tests were inherently testing the “both” style.

bbatsov · 2017-03-11T07:18:16Z

spec/rubocop/cop/style/negated_if_spec.rb

+      ].join("\n")
+    end
+
+    it 'does not autocorrect for postfix' do


Well, it if doesn't report offenses it won't auto-correct as well.

Ah! So it's not necessary (or even possible) to test the not case?

@Drenmi So I guess that means the conditionals don't need to be added to the autocorrect method, and that explains why the tests that I expected to fail passed, because if it only autocorrects when there's an offense, then the on_if method acts like a guard.

bbatsov · 2017-03-11T07:18:35Z

spec/rubocop/cop/style/negated_if_spec.rb

+      expect(corrected).to eq 'bar unless foo'
+    end
+
+    it 'does not autocorrect for prefix' do


bbatsov · 2017-03-11T07:19:55Z

spec/rubocop/cop/style/negated_if_spec.rb

@@ -95,6 +95,21 @@

      expect(cop.offenses).to be_empty
    end
+
+    it 'autocorrects by replacing if not with unless' do


Here probably there should be tests for both prefix and post auto-correction.

bbatsov · 2017-03-14T06:38:13Z

@brandonweiss ping :-)

brandonweiss · 2017-03-17T14:35:58Z

@bbatsov Sorry, I wasn't feeling well the last few days. I'm going to make these changes today!

[Fixes #3961] Some people have a strong preference against using unless with an `if` statement in multi-line/block form: ```ruby unless foo bar end ``` But they think it still makes sense to use it in single-line/modifier form: ```ruby bar unless foo ``` I’ve added three supported styles: * `prefix` will only enforce using `unless` in the multi-line/block form * `postfix` will only enforce using `unless` in the single-line/modifier form * `both` will enforce using `unless` in both the `prefix` and `postfix` forms

brandonweiss · 2017-03-19T05:43:04Z

Updated!

bbatsov · 2017-03-19T13:21:57Z

Thanks!

bbatsov reviewed Mar 11, 2017

View reviewed changes

bbatsov merged commit c332a94 into rubocop:master Mar 19, 2017

brandonweiss deleted the negated-if-with-prefix-postfix branch March 19, 2017 17:36

pocke mentioned this pull request Mar 20, 2017

Fix examples in the documentation for Style/SymbolArray cop #4147

Merged

dependabot bot mentioned this pull request Mar 12, 2021

Bump rubocop from 0.31.0 to 0.48.1 juslee/java-buildpack#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding “prefix” and “postfix” styles to Style/NegatedIf #4104

Adding “prefix” and “postfix” styles to Style/NegatedIf #4104

brandonweiss commented Mar 9, 2017 •

edited

Loading

brandonweiss commented Mar 9, 2017

brandonweiss commented Mar 9, 2017

brandonweiss commented Mar 10, 2017

brandonweiss commented Mar 10, 2017

bbatsov Mar 11, 2017

brandonweiss Mar 19, 2017

bbatsov Mar 11, 2017

brandonweiss Mar 19, 2017

bbatsov Mar 11, 2017

brandonweiss Mar 19, 2017

bbatsov Mar 11, 2017

brandonweiss Mar 19, 2017

bbatsov Mar 11, 2017

brandonweiss Mar 19, 2017

bbatsov Mar 11, 2017

brandonweiss Mar 19, 2017

bbatsov commented Mar 14, 2017

brandonweiss commented Mar 17, 2017

brandonweiss commented Mar 19, 2017

bbatsov commented Mar 19, 2017

Adding “prefix” and “postfix” styles to Style/NegatedIf #4104

Adding “prefix” and “postfix” styles to Style/NegatedIf #4104

Conversation

brandonweiss commented Mar 9, 2017 • edited Loading

brandonweiss commented Mar 9, 2017

brandonweiss commented Mar 9, 2017

brandonweiss commented Mar 10, 2017

brandonweiss commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Mar 14, 2017

brandonweiss commented Mar 17, 2017

brandonweiss commented Mar 19, 2017

bbatsov commented Mar 19, 2017

brandonweiss commented Mar 9, 2017 •

edited

Loading