[Fix #1232] make the behavior of AndOr cop configurable

Currently, the AndOr cop bans the use of keywords 'and' and 'or'
completely. This patch adds a configuration option `OnlyCheckConditionals`
which when set, restricts the AndOr cop to only conditionals.

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### New features
 
+* [#1232](https://github.com/bbatsov/rubocop/issues/1232): Add EnforcedStyle option to cop `AndOr` to restrict it to conditionals. ([@vrthra][])
 * [#835](https://github.com/bbatsov/rubocop/issues/835): New cop `PercentQLiterals` checks if use of `%Q` and `%q` matches configuration. ([@jonas054][])
 * [#835](https://github.com/bbatsov/rubocop/issues/835): New cop `BarePercentLiterals` checks if usage of `%()` or `%Q()` matches configuration. ([@jonas054][])
 * [#1079](https://github.com/bbatsov/rubocop/pull/1079): New cop `MultilineBlockLayout` checks if a multiline block has an extpression on the same line as the start of the block. ([@barunio][])
@@ -1041,3 +1042,4 @@
 [@camilleldn]: https://github.com/camilleldn
 [@mcls]: https://github.com/mcls
 [@yous]: https://github.com/yous
+[@vrthra]: https://github.com/vrthra

config/default.yml

@@ -108,6 +108,15 @@ Style/AlignParameters:
     - with_first_parameter
     - with_fixed_indentation
 
+Style/AndOr:
+  # Whether `and` and `or` are banned only in conditionals (conditionals)
+  # or completely (always).
+  EnforcedStyle: always
+  SupportedStyles:
+    - always
+    - conditionals
+
+
 # Checks if usage of %() or %Q() matches configuration.
 Style/BarePercentLiterals:
   EnforcedStyle: bare_percent

lib/rubocop/cop/style/and_or.rb

@@ -6,21 +6,40 @@ module Style
       # This cop checks for uses of *and* and *or*.
       class AndOr < Cop
         include AutocorrectUnlessChangingAST
+        include ConfigurableEnforcedStyle
 
         MSG = 'Use `%s` instead of `%s`.'
 
         OPS = { 'and' => '&&', 'or' => '||' }
 
         def on_and(node)
-          process_logical_op(node)
+          process_logical_op(node) if style == :always
         end
 
         def on_or(node)
-          process_logical_op(node)
+          process_logical_op(node) if style == :always
+        end
+
+        def on_if(node)
+          on_conditionals(node) if style == :conditionals
+        end
+
+        def on_while(node)
+          on_conditionals(node) if style == :conditionals
+        end
+
+        def on_until(node)
+          on_conditionals(node) if style == :conditionals
         end
 
         private
 
+        def on_conditionals(node)
+          on_node([:and, :or], node) do |my_node|
+            process_logical_op(my_node)
+          end
+        end
+
         def process_logical_op(node)
           op = node.loc.operator.source
           op_type = node.type.to_s

spec/rubocop/cop/style/and_or_spec.rb

@@ -2,56 +2,189 @@
 
 require 'spec_helper'
 
-describe RuboCop::Cop::Style::AndOr do
-  subject(:cop) { described_class.new }
-
-  it 'registers an offense for OR' do
-    inspect_source(cop,
-                   ['test if a or b'])
-    expect(cop.offenses.size).to eq(1)
-    expect(cop.messages).to eq(['Use `||` instead of `or`.'])
-  end
+describe RuboCop::Cop::Style::AndOr, :config do
+  context 'when style is conditionals' do
+    cop_config = {
+      'EnforcedStyle' => 'conditionals'
+    }
 
-  it 'registers an offense for AND' do
-    inspect_source(cop,
-                   ['test if a and b'])
-    expect(cop.offenses.size).to eq(1)
-    expect(cop.messages).to eq(['Use `&&` instead of `and`.'])
-  end
+    subject(:cop) { described_class.new(config) }
+    let(:cop_config) { cop_config }
 
-  it 'accepts ||' do
-    inspect_source(cop,
-                   ['test if a || b'])
-    expect(cop.offenses).to be_empty
-  end
+    it 'does not warn on short-circuit (and)' do
+      inspect_source(cop,
+                     ['x = a + b and return x'])
+      expect(cop.offenses.size).to eq(0)
+    end
 
-  it 'accepts &&' do
-    inspect_source(cop,
-                   ['test if a && b'])
-    expect(cop.offenses).to be_empty
-  end
+    it 'does not warn on short-circuit (or)' do
+      inspect_source(cop,
+                     ['x = a + b or return x'])
+      expect(cop.offenses.size).to eq(0)
+    end
 
-  it 'auto-corrects "and" with &&' do
-    new_source = autocorrect_source(cop, 'true and false')
-    expect(new_source).to eq('true && false')
-  end
+    it 'does warn on non short-circuit (and)' do
+      inspect_source(cop,
+                     ['x = a + b if a and b'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `&&` instead of `and`.'])
+    end
 
-  it 'auto-corrects "or" with ||' do
-    new_source = autocorrect_source(cop, ['x = 12345',
-                                          'true or false'])
-    expect(new_source).to eq(['x = 12345',
-                              'true || false'].join("\n"))
-  end
+    it 'does warn on non short-circuit (or)' do
+      inspect_source(cop,
+                     ['x = a + b if a or b'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `||` instead of `or`.'])
+    end
+
+    it 'does warn on non short-circuit (and) (unless)' do
+      inspect_source(cop,
+                     ['x = a + b unless a and b'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `&&` instead of `and`.'])
+    end
+
+    it 'does warn on non short-circuit (or) (unless)' do
+      inspect_source(cop,
+                     ['x = a + b unless a or b'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `||` instead of `or`.'])
+    end
+
+    it 'should handle boolean returning methods correctly' do
+      inspect_source(cop,
+                     ['1 if (not true) or false'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `||` instead of `or`.'])
+    end
+
+    it 'should handle recursion' do
+      inspect_source(cop,
+                     ['1 if (true and false) || (false or true)'])
+      expect(cop.offenses.size).to eq(2)
+    end
+
+    it 'should handle recursion' do
+      inspect_source(cop,
+                     ['1 if (true or false) && (false and true)'])
+      expect(cop.offenses.size).to eq(2)
+    end
 
-  it 'leaves *or* as is if auto-correction changes the meaning' do
-    src = "teststring.include? 'a' or teststring.include? 'b'"
-    new_source = autocorrect_source(cop, src)
-    expect(new_source).to eq(src)
   end
 
-  it 'leaves *and* as is if auto-correction changes the meaning' do
-    src = 'x = a + b and return x'
-    new_source = autocorrect_source(cop, src)
-    expect(new_source).to eq(src)
+  context 'when style is always' do
+    cop_config = {
+      'EnforcedStyle' => 'always'
+    }
+
+    subject(:cop) { described_class.new(config) }
+    let(:cop_config) { cop_config }
+
+    it 'registers an offense for OR' do
+      inspect_source(cop,
+                     ['test if a or b'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `||` instead of `or`.'])
+    end
+
+    it 'registers an offense for AND' do
+      inspect_source(cop,
+                     ['test if a and b'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `&&` instead of `and`.'])
+    end
+
+    it 'accepts ||' do
+      inspect_source(cop,
+                     ['test if a || b'])
+      expect(cop.offenses).to be_empty
+    end
+
+    it 'accepts &&' do
+      inspect_source(cop,
+                     ['test if a && b'])
+      expect(cop.offenses).to be_empty
+    end
+
+    it 'auto-corrects "and" with &&' do
+      new_source = autocorrect_source(cop, 'true and false')
+      expect(new_source).to eq('true && false')
+    end
+
+    it 'auto-corrects "or" with ||' do
+      new_source = autocorrect_source(cop, ['x = 12345',
+                                            'true or false'])
+      expect(new_source).to eq(['x = 12345',
+                                'true || false'].join("\n"))
+    end
+
+    it 'leaves *or* as is if auto-correction changes the meaning' do
+      src = "teststring.include? 'a' or teststring.include? 'b'"
+      new_source = autocorrect_source(cop, src)
+      expect(new_source).to eq(src)
+    end
+
+    it 'leaves *and* as is if auto-correction changes the meaning' do
+      src = 'x = a + b and return x'
+      new_source = autocorrect_source(cop, src)
+      expect(new_source).to eq(src)
+    end
+
+    it 'warns on short-circuit (and)' do
+      inspect_source(cop,
+                     ['x = a + b and return x'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `&&` instead of `and`.'])
+    end
+
+    it 'also warns on non short-circuit (and)' do
+      inspect_source(cop,
+                     ['x = a + b if a and b'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `&&` instead of `and`.'])
+    end
+
+    it 'also warns on non short-circuit (and) (unless)' do
+      inspect_source(cop,
+                     ['x = a + b unless a and b'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `&&` instead of `and`.'])
+    end
+
+    it 'warns on short-circuit (or)' do
+      inspect_source(cop,
+                     ['x = a + b or return x'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `||` instead of `or`.'])
+    end
+
+    it 'also warns on non short-circuit (or)' do
+      inspect_source(cop,
+                     ['x = a + b if a or b'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `||` instead of `or`.'])
+    end
+
+    it 'also warns on non short-circuit (or) (unless)' do
+      inspect_source(cop,
+                     ['x = a + b unless a or b'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `||` instead of `or`.'])
+    end
+
+    it 'also warns on while (or)' do
+      inspect_source(cop,
+                     ['x = a + b while a or b'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `||` instead of `or`.'])
+    end
+
+    it 'also warns on until (or)' do
+      inspect_source(cop,
+                     ['x = a + b until a or b'])
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages).to eq(['Use `||` instead of `or`.'])
+    end
+
   end
 end