Deprecate :bad status in favor of :fail

This naming convention was a holdover from very early on in Overcommit's history. However, much of the language used in Overcommit (e.g. the displayed output from the actual run, or the `fail_hook` matcher in specs) uses the verb `fail` instead of the adjective `bad`. We would like to standardize on using verbs going forward. Since there are currently custom hooks in the wild that are written to return `:bad`, we'll simply display an deprecation message for now that warns of the impending removal of this term. Change-Id: I6cbcce55e04d26bd6215449fb3427b135ab50099 Reviewed-on: http://gerrit.causes.com/40974 Tested-by: jenkins <jenkins@causes.com> Reviewed-by: Shane da Silva <shane.dasilva@brigade.com>
sds · Jul 28, 2014 · 1f51f11 · 1f51f11
1 parent ec35a52
commit 1f51f11
Show file tree

Hide file tree

Showing 32 changed files with 51 additions and 40 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,7 @@
 * Fix `overcommit-hook` auto-updating not passing original arguments to
   updated hook
 * Display message when `overcommit-hook` file is automatically updated
+* Deprecate `:bad` status in favor of `:fail`
 
 ## 0.14.1
 

diff --git a/README.md b/README.md
@@ -192,7 +192,7 @@ module Overcommit::Hook::PreCommit
       end
     end
 
-    return :bad, errors.join("\n") if errors.any?
+    return :fail, errors.join("\n") if errors.any?
 
     :good
   end

diff --git a/lib/overcommit/hook/commit_msg/gerrit_change_id.rb b/lib/overcommit/hook/commit_msg/gerrit_change_id.rb
@@ -12,7 +12,7 @@ def run
       result = execute([SCRIPT_LOCATION, commit_message_file])
       return :good if result.success?
 
-      [:bad, result.stdout]
+      [:fail, result.stdout]
     end
   end
 end
diff --git a/lib/overcommit/hook/pre_commit/author_email.rb b/lib/overcommit/hook/pre_commit/author_email.rb
@@ -6,9 +6,10 @@ def run
       email = result.stdout.chomp
 
       unless email =~ /#{@config['pattern']}/
-        return :bad, "Author has an invalid email address: '#{email}'\n" \
-                     'Set your email with ' \
-                     '`git config --global user.email your_email@example.com`'
+        return :fail,
+               "Author has an invalid email address: '#{email}'\n" \
+               'Set your email with ' \
+               '`git config --global user.email your_email@example.com`'
       end
 
       :good

diff --git a/lib/overcommit/hook/pre_commit/author_name.rb b/lib/overcommit/hook/pre_commit/author_name.rb
@@ -6,9 +6,9 @@ def run
       name = result.stdout.chomp
 
       unless name.split(' ').count >= 2
-        return :bad, 'Author must have at least first and last name, but ' \
-                     "was: '#{name}'.\nSet your name with " \
-                     "`git config --global user.name 'Your Name'`"
+        return :fail,
+               "Author must have at least first and last name, but was: #{name}." \
+               "\nSet your name with `git config --global user.name 'Your Name'`"
       end
 
       :good

diff --git a/lib/overcommit/hook/pre_commit/berksfile_check.rb b/lib/overcommit/hook/pre_commit/berksfile_check.rb
@@ -15,7 +15,7 @@ def run
 
       result = execute(%w[berks list --quiet])
       unless result.success?
-        return :bad, result.stderr
+        return :fail, result.stderr
       end
 
       :good

diff --git a/lib/overcommit/hook/pre_commit/brakeman.rb b/lib/overcommit/hook/pre_commit/brakeman.rb
@@ -10,7 +10,7 @@ def run
                        applicable_files)
       return :good if result.success?
 
-      [:bad, result.stdout]
+      [:fail, result.stdout]
     end
   end
 end
diff --git a/lib/overcommit/hook/pre_commit/bundle_check.rb b/lib/overcommit/hook/pre_commit/bundle_check.rb
@@ -15,12 +15,12 @@ def run
 
       result = execute(%w[bundle check])
       unless result.success?
-        return :bad, result.stdout
+        return :fail, result.stdout
       end
 
       result = execute(%w[git diff --quiet --] + [LOCK_FILE])
       unless result.success?
-        return :bad, "#{LOCK_FILE} is not up-to-date -- run `bundle check`"
+        return :fail, "#{LOCK_FILE} is not up-to-date -- run `bundle check`"
       end
 
       :good

diff --git a/lib/overcommit/hook/pre_commit/chamber_security.rb b/lib/overcommit/hook/pre_commit/chamber_security.rb
@@ -9,7 +9,7 @@ def run
       result = execute(%w[chamber secure --echo --files] + applicable_files)
 
       return :good if result.stdout.empty?
-      [:bad, "These settings appear to need to be secured but were not: #{result.stdout}"]
+      [:fail, "These settings appear to need to be secured but were not: #{result.stdout}"]
     end
   end
 end
diff --git a/lib/overcommit/hook/pre_commit/coffee_lint.rb b/lib/overcommit/hook/pre_commit/coffee_lint.rb
@@ -9,7 +9,7 @@ def run
       result = execute(%w[coffeelint --quiet] + applicable_files)
       return :good if result.success?
 
-      [:bad, result.stdout]
+      [:fail, result.stdout]
     end
   end
 end
diff --git a/lib/overcommit/hook/pre_commit/css_lint.rb b/lib/overcommit/hook/pre_commit/css_lint.rb
@@ -9,7 +9,7 @@ def run
       result = execute(%w[csslint --quiet --format=compact] + applicable_files)
       return :good if result.stdout !~ /Error - (?!Unknown @ rule)/
 
-      [:bad, result.stdout]
+      [:fail, result.stdout]
     end
   end
 end
diff --git a/lib/overcommit/hook/pre_commit/go_lint.rb b/lib/overcommit/hook/pre_commit/go_lint.rb
@@ -10,7 +10,7 @@ def run
       # Unfortunately the exit code is always 0
       return :good if result.stdout.empty?
 
-      [:bad, result.stdout]
+      [:fail, result.stdout]
     end
   end
 end
diff --git a/lib/overcommit/hook/pre_commit/haml_lint.rb b/lib/overcommit/hook/pre_commit/haml_lint.rb
@@ -18,7 +18,7 @@ def run
         modified_lines(file).include?(line.to_i)
       end
 
-      return :bad, error_lines.join("\n") unless error_lines.empty?
+      return :fail, error_lines.join("\n") unless error_lines.empty?
 
       [:warn, "Modified files have lints (on lines you didn't modify)\n" <<
               warning_lines.join("\n")]

diff --git a/lib/overcommit/hook/pre_commit/hard_tabs.rb b/lib/overcommit/hook/pre_commit/hard_tabs.rb
@@ -5,7 +5,7 @@ def run
       # Catches hard tabs
       result = execute(%w[grep -IHn] + ["\t"] + applicable_files)
       unless result.stdout.empty?
-        return :bad, "Hard tabs detected:\n#{result.stdout}"
+        return :fail, "Hard tabs detected:\n#{result.stdout}"
       end
 
       :good

diff --git a/lib/overcommit/hook/pre_commit/image_optim.rb b/lib/overcommit/hook/pre_commit/image_optim.rb
@@ -12,11 +12,11 @@ def run
         begin
           optimize_images(applicable_files)
         rescue ::ImageOptim::BinNotFoundError => e
-          return :bad, "#{e.message}. The image_optim gem is dependendent on this binary."
+          return :fail, "#{e.message}. The image_optim gem is dependendent on this binary."
         end
 
       if optimized_images.any?
-        return :bad,
+        return :fail,
           "The following images are optimizable:\n#{optimized_images.join("\n")}" \
           "\n\nOptimize them by running:\n" \
           "  image_optim #{optimized_images.join(' ')}"

diff --git a/lib/overcommit/hook/pre_commit/js_hint.rb b/lib/overcommit/hook/pre_commit/js_hint.rb
@@ -11,7 +11,7 @@ def run
 
       return :good if output.empty?
 
-      [:bad, output]
+      [:fail, output]
     end
   end
 end
diff --git a/lib/overcommit/hook/pre_commit/jscs.rb b/lib/overcommit/hook/pre_commit/jscs.rb
@@ -23,7 +23,7 @@ def run
         modified_lines(file).include?(line.to_i)
       end
 
-      return :bad, error_lines.join("\n") unless error_lines.empty?
+      return :fail, error_lines.join("\n") unless error_lines.empty?
 
       [:warn, "Modified files have lints (on lines you didn't modify)\n" <<
               warning_lines.join("\n")]

diff --git a/lib/overcommit/hook/pre_commit/json_syntax.rb b/lib/overcommit/hook/pre_commit/json_syntax.rb
@@ -16,7 +16,7 @@ def run
 
       return :good if output.empty?
 
-      [:bad, output]
+      [:fail, output]
     end
   end
 end
diff --git a/lib/overcommit/hook/pre_commit/jsx_hint.rb b/lib/overcommit/hook/pre_commit/jsx_hint.rb
@@ -11,7 +11,7 @@ def run
 
       return :good if output.empty?
 
-      [:bad, output]
+      [:fail, output]
     end
   end
 end
diff --git a/lib/overcommit/hook/pre_commit/merge_conflicts.rb b/lib/overcommit/hook/pre_commit/merge_conflicts.rb
@@ -5,7 +5,7 @@ def run
       result = execute(%w[grep -IHn ^<<<<<<<\s] + applicable_files)
 
       unless result.stdout.empty?
-        return :bad, "Merge conflict markers detected:\n#{result.stdout}"
+        return :fail, "Merge conflict markers detected:\n#{result.stdout}"
       end
 
       :good

diff --git a/lib/overcommit/hook/pre_commit/pry_binding.rb b/lib/overcommit/hook/pre_commit/pry_binding.rb
@@ -5,7 +5,7 @@ def run
       result = execute(%w[grep -IHnE ^\s*binding\.pry] + applicable_files)
 
       unless result.stdout.empty?
-        return :bad, "Found a `binding.pry` call left in:\n#{result.stdout}"
+        return :fail, "Found a `binding.pry` call left in:\n#{result.stdout}"
       end
 
       :good

diff --git a/lib/overcommit/hook/pre_commit/python_flake8.rb b/lib/overcommit/hook/pre_commit/python_flake8.rb
@@ -9,7 +9,7 @@ def run
       result = execute(%w[flake8] + applicable_files)
       return :good if result.success?
 
-      [:bad, result.stdout]
+      [:fail, result.stdout]
     end
   end
 end
diff --git a/lib/overcommit/hook/pre_commit/rails_schema_up_to_date.rb b/lib/overcommit/hook/pre_commit/rails_schema_up_to_date.rb
@@ -3,16 +3,16 @@ module Overcommit::Hook::PreCommit
   class RailsSchemaUpToDate < Base
     def run # rubocop:disable CyclomaticComplexity
       if migration_files.any? && schema_files.none?
-        return :bad, "It looks like you're adding a migration, but did not update the schema file"
+        return :fail, "It looks like you're adding a migration, but did not update the schema file"
       elsif migration_files.none? && schema_files.any?
-        return :bad, "You're trying to change the schema without adding a migration file"
+        return :fail, "You're trying to change the schema without adding a migration file"
       elsif migration_files.any? && schema_files.any?
         latest_version = migration_files.map  { |file| file[/\d+/] }.sort.last
         schema         = schema_files.map     { |file| File.read(file) }.join
         up_to_date     = schema.include?(latest_version)
 
         unless up_to_date
-          return :bad, "The latest migration version you're committing is " \
+          return :fail, "The latest migration version you're committing is " \
                        "#{latest_version}, but your schema file " \
                        "#{schema_files.join(' or ')} is on a different version."
         end

diff --git a/lib/overcommit/hook/pre_commit/rubocop.rb b/lib/overcommit/hook/pre_commit/rubocop.rb
@@ -20,7 +20,7 @@ def run
         modified_lines(file).include?(line.to_i)
       end
 
-      return :bad, error_lines.join("\n") unless error_lines.empty?
+      return :fail, error_lines.join("\n") unless error_lines.empty?
 
       [:warn, "Modified files have lints (on lines you didn't modify)\n" <<
               warning_lines.join("\n")]

diff --git a/lib/overcommit/hook/pre_commit/scss_lint.rb b/lib/overcommit/hook/pre_commit/scss_lint.rb
@@ -18,7 +18,7 @@ def run
         modified_lines(file).include?(line.to_i)
       end
 
-      return :bad, error_lines.join("\n") unless error_lines.empty?
+      return :fail, error_lines.join("\n") unless error_lines.empty?
 
       [:warn, "Modified files have lints (on lines you didn't modify)\n" <<
               warning_lines.join("\n")]

diff --git a/lib/overcommit/hook/pre_commit/trailing_whitespace.rb b/lib/overcommit/hook/pre_commit/trailing_whitespace.rb
@@ -4,7 +4,7 @@ class TrailingWhitespace < Base
     def run
       result = execute(%w[grep -IHn \s$] + applicable_files)
       unless result.stdout.empty?
-        return :bad, "Trailing whitespace detected:\n#{result.stdout}"
+        return :fail, "Trailing whitespace detected:\n#{result.stdout}"
       end
 
       :good

diff --git a/lib/overcommit/hook/pre_commit/travis_lint.rb b/lib/overcommit/hook/pre_commit/travis_lint.rb
@@ -9,7 +9,7 @@ def run
       result = execute(%w[travis-lint] + applicable_files)
       return :good if result.success?
 
-      [:bad, result.stdout.strip]
+      [:fail, result.stdout.strip]
     end
   end
 end
diff --git a/lib/overcommit/hook/pre_commit/yaml_syntax.rb b/lib/overcommit/hook/pre_commit/yaml_syntax.rb
@@ -16,7 +16,7 @@ def run
 
       return :good if output.empty?
 
-      [:bad, output]
+      [:fail, output]
     end
   end
 end
diff --git a/lib/overcommit/hook_runner.rb b/lib/overcommit/hook_runner.rb
@@ -40,7 +40,7 @@ def run_hooks
         @hooks.each do |hook|
           hook_status = run_hook(hook)
 
-          run_failed = true if hook_status == :bad
+          run_failed = true if [:bad, :fail].include?(hook_status)
 
           if hook_status == :interrupted
             # Stop running any more hooks and assume a bad result
@@ -70,7 +70,7 @@ def run_hook(hook)
         InterruptHandler.disable!
         status, output = hook.run
       rescue => ex
-        status = :bad
+        status = :fail
         output = "Hook raised unexpected error\n#{ex.message}"
       rescue Interrupt
         status = :interrupted

diff --git a/lib/overcommit/printer.rb b/lib/overcommit/printer.rb
@@ -66,19 +66,28 @@ def print_header(hook)
       log.partial '.' * (70 - hook.description.length)
     end
 
-    def print_result(hook, status, output)
+    def print_result(hook, status, output) # rubocop:disable CyclomaticComplexity, MethodLength
       case status
       when :good
         log.success 'OK' unless hook.quiet?
       when :warn
         log.warning 'WARNING'
         print_report(output, :bold_warning)
       when :bad
+        log.error 'FAILED'
+        log.bold_error 'Hook returned a status of `:bad`. This is deprecated ' \
+                       'in favor of `:fail` and will be removed in a future ' \
+                       'version of Overcommit'
+        print_report(output, :bold_error)
+      when :fail
         log.error 'FAILED'
         print_report(output, :bold_error)
       when :interrupted
         log.error 'INTERRUPTED'
         print_report(output, :bold_error)
+      else
+        log.error '???'
+        log.bold_error "Hook returned unknown status `#{status.inspect}` -- ignoring." \
       end
     end
 

diff --git a/spec/overcommit/hook_signer_spec.rb b/spec/overcommit/hook_signer_spec.rb
@@ -58,7 +58,7 @@ def run
         module Overcommit::Hook::PreCommit
           class SomeHook
             def run
-              :bad # This line changed
+              :fail # This line changed
             end
           end
         end

diff --git a/spec/support/matchers/hook.rb b/spec/support/matchers/hook.rb
@@ -40,7 +40,7 @@ def failure_message(actual, error_message)
 
 # Can't use 'fail' as it is a reserved word.
 RSpec::Matchers.define :fail_hook  do |*args|
-  check_matcher = HookMatcher.new(:bad, args)
+  check_matcher = HookMatcher.new(:fail, args)
 
   match do
     check_matcher.matches?(actual)
-Original file line number
+Diff line change
@@ Expand Up / @@ -192,7 +192,7 @@ module Overcommit::Hook::PreCommit @@
           end
         end
-        return :bad, errors.join("\n") if errors.any?
+        return :fail, errors.join("\n") if errors.any?
         :good
       end
@@ Expand Down @@