Merge pull request #137 from RichardBradley/not_rescue_exception

Disallow catching 'Exception'

lib/rails_best_practices/reviews.rb

@@ -33,3 +33,4 @@
 require 'rails_best_practices/reviews/protect_mass_assignment_review'
 require 'rails_best_practices/reviews/use_parentheses_in_method_def_review'
 require 'rails_best_practices/reviews/hash_syntax_review'
+require 'rails_best_practices/reviews/not_rescue_exception_review'

lib/rails_best_practices/reviews/not_rescue_exception_review.rb

@@ -0,0 +1,39 @@
+# encoding: utf-8
+require 'rails_best_practices/reviews/review'
+
+module RailsBestPractices
+  module Reviews
+    # Review all code to make sure we don't rescue Exception
+    # This is a common mistake by Java or C# devs in ruby.
+    #
+    # See the best practice details here http://stackoverflow.com/questions/10048173/why-is-it-bad-style-to-rescue-exception-e-in-ruby
+    #
+    # Implementation:
+    #
+    # Review process:
+    #   check all rescue node to see if its type is Exception
+    class NotRescueExceptionReview < Review
+      interesting_nodes :rescue
+      interesting_files ALL_FILES
+      url "http://stackoverflow.com/questions/10048173/why-is-it-bad-style-to-rescue-exception-e-in-ruby"
+
+      # check rescue node to see if its type is Exception
+      add_callback :start_rescue do |node|
+        rescue_args = node[1]
+        if rescue_args
+          rescue_type = rescue_args.first
+          if rescue_type && rescue_type.first == :var_ref
+            rescue_type_var = rescue_type[1]
+            if rescue_type_var.first == :@const
+              if "Exception" == rescue_type_var[1]
+                # 'rescue' nodes do not have line-number info, but the rescue_type
+                # node does.
+                add_error "not rescue Exception", node.file, rescue_type.line
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

rails_best_practices.yml

@@ -12,6 +12,7 @@ MoveCodeIntoModelCheck: { use_count: 2 }
 MoveFinderToNamedScopeCheck: { }
 MoveModelLogicIntoModelCheck: { use_count: 4 }
 NeedlessDeepNestingCheck: { nested_count: 2 }
+NotRescueExceptionCheck: { }
 NotUseDefaultRouteCheck: { }
 NotUseTimeAgoInWordsCheck: { }
 OveruseRouteCustomizationsCheck: { customize_count: 3 }

spec/rails_best_practices/reviews/not_rescue_exception_spec.rb

@@ -0,0 +1,95 @@
+require 'spec_helper'
+
+module RailsBestPractices
+  module Reviews
+    describe NotRescueExceptionReview do
+      let(:runner) { Core::Runner.new(reviews: NotRescueExceptionReview.new) }
+
+      describe "not_rescue_exception" do
+        it "should not rescue exception in method rescue with named var" do
+          content =<<-EOF
+          def my_method
+            do_something
+          rescue Exception => e
+            logger.error e
+          end
+          EOF
+          runner.review('app/helpers/posts_helper.rb', content)
+          runner.should have(1).errors
+          runner.errors[0].to_s.should == "app/helpers/posts_helper.rb:3 - not rescue Exception"
+        end
+
+        it "should not rescue exception in method rescue without named var" do
+          content =<<-EOF
+          def my_method
+            do_something
+          rescue Exception
+            logger.error $!
+          end
+          EOF
+          runner.review('app/helpers/posts_helper.rb', content)
+          runner.should have(1).errors
+          runner.errors[0].to_s.should == "app/helpers/posts_helper.rb:3 - not rescue Exception"
+        end
+
+        it "should not rescue exception in block rescue with named var" do
+          content =<<-EOF
+          def my_method
+            begin
+              do_something
+            rescue Exception => e
+              logger.error e
+            end
+          end
+          EOF
+          runner.review('app/helpers/posts_helper.rb', content)
+          runner.should have(1).errors
+          runner.errors[0].to_s.should == "app/helpers/posts_helper.rb:4 - not rescue Exception"
+        end
+
+        it "should not rescue exception in block rescue without named var" do
+          content =<<-EOF
+          def my_method
+            begin
+              do_something
+            rescue Exception
+              logger.error $!
+            end
+          end
+          EOF
+          runner.review('app/helpers/posts_helper.rb', content)
+          runner.should have(1).errors
+          runner.errors[0].to_s.should == "app/helpers/posts_helper.rb:4 - not rescue Exception"
+        end
+
+        it "should allow rescue implicit StandardError in block rescue without named var" do
+          content =<<-EOF
+          def my_method
+            begin
+              do_something
+            rescue
+              logger.error $!
+            end
+          end
+          EOF
+          runner.review('app/helpers/posts_helper.rb', content)
+          runner.should have(0).errors
+        end
+
+        it "should allow rescue explicit StandardError in block rescue without named var" do
+          content =<<-EOF
+          def my_method
+            begin
+              do_something
+            rescue StandardError
+              logger.error $!
+            end
+          end
+          EOF
+          runner.review('app/helpers/posts_helper.rb', content)
+          runner.should have(0).errors
+        end
+      end
+    end
+  end
+end