Refactor Cask's doctor command

- Remove :failed
- Use Checkable to store the status and warnings/errors
- Refactor the methods using audit as basis

1 files changed, 114 insertions, 13 deletions

diff --git a/Library/Homebrew/cask/lib/hbc/cli/doctor.rb b/Library/Homebrew/cask/lib/hbc/cli/doctor.rb
index e439c871b..ff4a37bcf 100644
--- a/Library/Homebrew/cask/lib/hbc/cli/doctor.rb
+++ b/Library/Homebrew/cask/lib/hbc/cli/doctor.rb
@@ -1,9 +1,10 @@
 require "system_config"
+require "hbc/checkable"
 
 module Hbc
   class CLI
     class Doctor < AbstractCommand
-      attr_accessor :failed
+      include Checkable
 
       def initialize(*)
         super
@@ -11,16 +12,103 @@ module Hbc
         raise ArgumentError, "#{self.class.command_name} does not take arguments."
       end
 
+      def success?
+        !(errors? || warnings?)
+      end
+
+      def summary_header
+        "Cask's Doctor checkup"
+      end
+
       def run
+        check_software_versions
+        check_install_location
+        check_staging_location
+        check_taps
+        check_load_path
+        check_environment_variables
+
+        puts summary unless success?
+        raise CaskError, "Your system is not ready for Cask." unless success?
+      end
+
+      def check_software_versions
         ohai "Homebrew-Cask Version", Hbc.full_version
         ohai "macOS", MacOS.full_version
         ohai "Java", SystemConfig.describe_java
-        ohai "Homebrew-Cask Install Location", self.class.render_install_location
-        ohai "Homebrew-Cask Staging Location", self.class.render_staging_location(Hbc.caskroom)
-        ohai "Homebrew-Cask Cached Downloads", self.class.render_cached_downloads
+      end
+
+      # This could be done by calling into Homebrew, but the situation
+      # where "doctor" is needed is precisely the situation where such
+      # things are less dependable.
+      def check_install_location
+        ohai "Homebrew-Cask Install Location"
+
+        locations = Dir.glob(HOMEBREW_CELLAR.join("brew-cask", "*")).reverse
+        if locations.empty?
+          puts self.class.none_string
+        else
+          locations.collect do |l|
+            add_error "Legacy install at #{l}. Run \"brew uninstall --force brew-cask\"."
+            puts l
+          end
+        end
+      end
+
+      def check_staging_location
+        ohai "Homebrew-Cask Staging Location"
+
+        path = Pathname.new(user_tilde(Hbc.caskroom.to_s))
+
+        if !path.exist?
+          add_error "The staging path #{path} does not exist."
+        elsif !path.writable?
+          add_error "The staging path #{path} is not writable by the current user."
+        end
+
+        puts path
+      end
+
+      def check_cached_downloads
+        ohai "Homebrew-Cask Cached Downloads"
+
+        cleanup = CLI::Cleanup.new
+        count = cleanup.cache_files.count
+        size = cleanup.disk_cleanup_size
+        msg = user_tilde(Hbc.cache.to_s)
+        msg << " (#{number_readable(count)} files, #{disk_usage_readable(size)})" unless count.zero?
+        puts msg
+      end
+
+      def check_taps
         ohai "Homebrew-Cask Taps:"
-        puts self.class.render_taps(Hbc.default_tap, *self.class.alt_taps)
-        ohai "Contents of $LOAD_PATH", self.class.render_load_path($LOAD_PATH)
+
+        default_tap = [Hbc.default_tap]
+
+        alt_taps = Tap.select { |t| t.cask_dir.exist? && t != Hbc.default_tap }
+
+        (default_tap + alt_taps).each do |tap|
+          if tap.path.nil? || tap.path.to_s.empty?
+            puts none_string
+          else
+            puts "#{tap.path} (#{cask_count_for_tap(tap)})"
+          end
+        end
+      end
+
+      def check_load_path
+        ohai "Contents of $LOAD_PATH"
+        paths = $LOAD_PATH.map(&method(:user_tilde))
+
+        if paths.empty?
+          puts none_string
+          add_error "$LOAD_PATH is empty"
+        else
+          puts paths
+        end
+      end
+
+      def check_environment_variables
         ohai "Environment Variables"
 
         environment_variables = %w[
@@ -36,9 +124,25 @@ module Hbc
           SHELL
         ]
 
-        (self.class.locale_variables + environment_variables).sort.each(&self.class.method(:render_env_var))
+        locale_variables = ENV.keys.grep(/^(?:LC_\S+|LANG|LANGUAGE)\Z/).sort
+
+        (locale_variables + environment_variables).sort.each(&method(:render_env_var))
+      end
+
+      def user_tilde(path)
+        self.class.user_tilde(path)
+      end
+
+      def cask_count_for_tap(tap)
+        self.class.cask_count_for_tap(tap)
+      end
+
+      def none_string
+        self.class.none_string
+      end
 
-        raise CaskError, "One or more checks failed." if @failed
+      def render_env_var(var)
+        self.class.render_env_var(var)
       end
 
       def self.locale_variables
@@ -65,8 +169,8 @@ module Hbc
       def self.cask_count_for_tap(tap)
         Formatter.pluralize(tap.cask_files.count, "cask")
       rescue StandardError
-        @failed = true
-        "0 #{error_string "error reading #{tap.path}"}"
+        add_error "Unable to read from Tap: #{tap.path}"
+        "0"
       end
 
       def self.render_taps(*taps)
@@ -97,7 +201,6 @@ module Hbc
         if locations.empty?
           none_string
         else
-          @failed = true
           locations.collect do |l|
             "#{l} #{error_string 'error: legacy install. Run "brew uninstall --force brew-cask".'}"
           end
@@ -107,10 +210,8 @@ module Hbc
       def self.render_staging_location(path)
         path = Pathname.new(user_tilde(path.to_s))
         if !path.exist?
-          @failed = true
           "#{path} #{error_string "error: path does not exist"}"
         elsif !path.writable?
-          @failed = true
           "#{path} #{error_string "error: not writable by current user"}"
         else
           path