From ae4316adb1de463e8b641550d79f1be39b52b2ef Mon Sep 17 00:00:00 2001 From: Jamie Lawrence Date: Wed, 20 Apr 2016 13:43:06 +0100 Subject: [PATCH 1/3] Added short circuit check Based on the 'debugger' check, this checks for accidental short-circuits often used in debugging. ``` if true && something if false && something ``` --- .../pre_commit/checks/short_circuit.rb | 20 +++++++++++++++++ test/files/shortcircuit_file.rb | 11 ++++++++++ .../pre_commit/checks/shortcircuit_test.rb | 22 +++++++++++++++++++ 3 files changed, 53 insertions(+) create mode 100644 lib/plugins/pre_commit/checks/short_circuit.rb create mode 100644 test/files/shortcircuit_file.rb create mode 100644 test/unit/plugins/pre_commit/checks/shortcircuit_test.rb diff --git a/lib/plugins/pre_commit/checks/short_circuit.rb b/lib/plugins/pre_commit/checks/short_circuit.rb new file mode 100644 index 0000000..2b61d00 --- /dev/null +++ b/lib/plugins/pre_commit/checks/short_circuit.rb @@ -0,0 +1,20 @@ +require 'pre-commit/checks/grep' + +module PreCommit + module Checks + class ShortCircuit < Grep + def message + "Logical short circuit found:" + end + + def pattern + "^.*([true|false] \&\&)" + end + + def self.description + "Finds files with a logical short circuit like 'if true &&'" + end + + end + end +end diff --git a/test/files/shortcircuit_file.rb b/test/files/shortcircuit_file.rb new file mode 100644 index 0000000..fd09239 --- /dev/null +++ b/test/files/shortcircuit_file.rb @@ -0,0 +1,11 @@ +class ShortCircuitFile + def blam + if true && something + puts 'this will always print' + end + + if false && something + puts 'this will never print' + end + end +end diff --git a/test/unit/plugins/pre_commit/checks/shortcircuit_test.rb b/test/unit/plugins/pre_commit/checks/shortcircuit_test.rb new file mode 100644 index 0000000..da864a6 --- /dev/null +++ b/test/unit/plugins/pre_commit/checks/shortcircuit_test.rb @@ -0,0 +1,22 @@ +require 'minitest_helper' +require 'plugins/pre_commit/checks/short_circuit' + +describe PreCommit::Checks::ShortCircuit do + subject{ PreCommit::Checks::ShortCircuit.new(nil, nil, []) } + + it "succeeds if nothing changed" do + subject.call([]).must_equal nil + end + + it "succeeds if only good changes" do + subject.call([fixture_file('valid_file.rb')]).must_equal nil + end + + it "fails if file contains true &&" do + subject.call([fixture_file('shortcircuit_file.rb')]).to_a.must_equal([ + "Logical short circuit found:", + "test/files/shortcircuit_file.rb:3: if true && something", + "test/files/shortcircuit_file.rb:7: if false && something" + ]) + end +end From 56d7445fcbaed3d8fbbaf4a23a753b68d7874161 Mon Sep 17 00:00:00 2001 From: Jamie Lawrence Date: Wed, 20 Apr 2016 16:03:51 +0100 Subject: [PATCH 2/3] Update shortcircuit detection --- .../pre_commit/checks/short_circuit.rb | 5 +- test/files/shortcircuit_file.rb | 20 ++++++- .../pre_commit/checks/short_circuit_test.rb | 57 +++++++++++++++++++ .../pre_commit/checks/shortcircuit_test.rb | 22 ------- 4 files changed, 77 insertions(+), 27 deletions(-) create mode 100644 test/unit/plugins/pre_commit/checks/short_circuit_test.rb delete mode 100644 test/unit/plugins/pre_commit/checks/shortcircuit_test.rb diff --git a/lib/plugins/pre_commit/checks/short_circuit.rb b/lib/plugins/pre_commit/checks/short_circuit.rb index 2b61d00..865e5ea 100644 --- a/lib/plugins/pre_commit/checks/short_circuit.rb +++ b/lib/plugins/pre_commit/checks/short_circuit.rb @@ -8,11 +8,12 @@ def message end def pattern - "^.*([true|false] \&\&)" + "(false \&\&)|(true \\|\\|)" + # "(false\s*\&\&|true\s*\|\|)" end def self.description - "Finds files with a logical short circuit like 'if true &&'" + "Finds files with a logical short circuit like 'if false &&' or 'true ||'" end end diff --git a/test/files/shortcircuit_file.rb b/test/files/shortcircuit_file.rb index fd09239..f1849ca 100644 --- a/test/files/shortcircuit_file.rb +++ b/test/files/shortcircuit_file.rb @@ -1,11 +1,25 @@ class ShortCircuitFile def blam - if true && something + if false && something + puts 'this will never print' + end + + if true || something puts 'this will always print' end - if false && something - puts 'this will never print' + # These are daft but ok + if false || something + puts 'daft but valid' end + + if true && something + puts 'daft but valid' + end + + if true + puts "this will always print" + end + end end diff --git a/test/unit/plugins/pre_commit/checks/short_circuit_test.rb b/test/unit/plugins/pre_commit/checks/short_circuit_test.rb new file mode 100644 index 0000000..776bdea --- /dev/null +++ b/test/unit/plugins/pre_commit/checks/short_circuit_test.rb @@ -0,0 +1,57 @@ +require 'minitest_helper' +require 'plugins/pre_commit/checks/short_circuit' + +describe PreCommit::Checks::ShortCircuit do + subject{ PreCommit::Checks::ShortCircuit.new(nil, nil, []) } + + it "succeeds if nothing changed" do + subject.call([]).must_equal nil + end + + it "succeeds if only good changes" do + subject.call([fixture_file('valid_file.rb')]).must_equal nil + end + + it "fails if file contains true ||" do + subject.call([fixture_file('shortcircuit_file.rb')]).to_a.must_include( + "test/files/shortcircuit_file.rb:3: if false && something" + ) + end + + it "fails if file contains true ||" do + subject.call([fixture_file('shortcircuit_file.rb')]).to_a.must_include( + "test/files/shortcircuit_file.rb:7: if true || something" + ) + end + + it "fails if file contains false &&" do + subject.call([fixture_file('shortcircuit_file.rb')]).to_a.must_include( + "test/files/shortcircuit_file.rb:3: if false && something" + ) + end + + it "includes the error description" do + subject.call([fixture_file('shortcircuit_file.rb')]).to_a.must_include( + "Logical short circuit found:" + ) + end + + # Daft but not 'wrong' + it "succeeds if file contains false ||" do + subject.call([fixture_file('shortcircuit_file.rb')]).to_a.wont_include( + "test/files/shortcircuit_file.rb:12: if false || something" + ) + end + + it "succeeds if file contains true &&" do + subject.call([fixture_file('shortcircuit_file.rb')]).to_a.wont_include( + "test/files/shortcircuit_file.rb:16: if true && something" + ) + end + + it "succeeds if file contains true" do + subject.call([fixture_file('shortcircuit_file.rb')]).to_a.wont_include( + "test/files/shortcircuit_file.rb:20: if true" + ) + end +end diff --git a/test/unit/plugins/pre_commit/checks/shortcircuit_test.rb b/test/unit/plugins/pre_commit/checks/shortcircuit_test.rb deleted file mode 100644 index da864a6..0000000 --- a/test/unit/plugins/pre_commit/checks/shortcircuit_test.rb +++ /dev/null @@ -1,22 +0,0 @@ -require 'minitest_helper' -require 'plugins/pre_commit/checks/short_circuit' - -describe PreCommit::Checks::ShortCircuit do - subject{ PreCommit::Checks::ShortCircuit.new(nil, nil, []) } - - it "succeeds if nothing changed" do - subject.call([]).must_equal nil - end - - it "succeeds if only good changes" do - subject.call([fixture_file('valid_file.rb')]).must_equal nil - end - - it "fails if file contains true &&" do - subject.call([fixture_file('shortcircuit_file.rb')]).to_a.must_equal([ - "Logical short circuit found:", - "test/files/shortcircuit_file.rb:3: if true && something", - "test/files/shortcircuit_file.rb:7: if false && something" - ]) - end -end From 4fe1a95a933c3ebe0808e70d05cec545400c95b7 Mon Sep 17 00:00:00 2001 From: Jamie Lawrence Date: Wed, 20 Apr 2016 16:48:38 +0100 Subject: [PATCH 3/3] Remove old code --- lib/plugins/pre_commit/checks/short_circuit.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/plugins/pre_commit/checks/short_circuit.rb b/lib/plugins/pre_commit/checks/short_circuit.rb index 865e5ea..bc0a404 100644 --- a/lib/plugins/pre_commit/checks/short_circuit.rb +++ b/lib/plugins/pre_commit/checks/short_circuit.rb @@ -8,8 +8,7 @@ def message end def pattern - "(false \&\&)|(true \\|\\|)" - # "(false\s*\&\&|true\s*\|\|)" + "(false\s*\&\&)|(true\s*\\|\\|)" end def self.description