[Pod::Command::Spec] Refactoring lint logic

lib/cocoapods/command/spec.rb

@@ -26,18 +26,17 @@ module Pod
       def initialize(argv)
         @action = argv.shift_argument
         if @action == 'create'
-          @name_or_url = argv.shift_argument
-          @url = argv.shift_argument
+          @name_or_url     = argv.shift_argument
+          @url             = argv.shift_argument
           super if @name_or_url.nil?
         elsif @action == 'lint'
-          @quick  = argv.option('--quick')
-          @only_errors = argv.option('--only-errors')
+          @quick           = argv.option('--quick')
+          @only_errors     = argv.option('--only-errors')
           @repo_or_podspec = argv.shift_argument unless argv.empty?
           super unless argv.size <= 1
         else
           super
         end
-
         super unless argv.empty?
       end
 
@@ -61,109 +60,166 @@ module Pod
       end
 
       def lint
-        if (is_repo = repo_with_name_exist(@repo_or_podspec))
-          files = (config.repos_dir + @repo_or_podspec).glob('**/*.podspec')
+        puts
+        all_valid = lint_podspecs
+        if all_valid
+          puts lint_passed_message unless config.silent?
+        else
+          raise Informative, lint_failed_message
+        end
+      end
+
+      private
+
+      def lint_podspecs
+        all_valid = true
+        podspecs_to_lint.each do |podspec_file|
+          root_spec = Specification.from_file(podspec_file)
+          specs = root_spec.recursive_subspecs.any? ? root_spec.recursive_subspecs : [root_spec]
+          specs.each do |spec|
+            # Show immediatly which pod is being processed.
+            print " -> #{spec}\r" unless config.silent? || is_repo?
+            $stdout.flush
+
+            linter = Linter.new(spec, podspec_file, is_repo?, @quick, @only_errors)
+            all_valid = linter.lint && all_valid
+
+            # This overwrites the previously printed text
+            puts " -> ".send(lint_result_color(linter)) << spec.to_s unless config.silent? || should_skip?(linter)
+            print_messages(spec, 'ERROR', linter.errors)
+            print_messages(spec, 'WARN',  linter.warnings)
+            print_messages(spec, 'NOTE',  linter.notes)
+            puts unless config.silent? || should_skip?(linter)
+          end
+        end
+        all_valid
+      end
+
+      def lint_result_color(linter)
+        if linter.errors.empty? && linter.warnings.empty?
+          :green
+        elsif linter.errors.empty?
+          :yellow
         else
-          if @repo_or_podspec
+          :red
+        end
+      end
+
+      def should_skip?(linter)
+        is_repo? && linter.errors.empty? && linter.warnings.empty? && linter.notes.empty?
+      end
+
+      def print_messages(spec, type, messages)
+        return if config.silent?
+        if spec.platform.name
+          messages = clean_platfrom_messages(messages)
+        else
+          messages = clean_duplicate_platfrom_messages(messages)
+        end
+        messages.each {|msg| puts "    - #{type.ljust(5)} | #{msg}"}
+      end
+
+      def clean_platfrom_messages(messages)
+        messages.map { |l| l.gsub(/ios: /,'').gsub(/osx: /,'') }
+      end
+
+      def clean_duplicate_platfrom_messages(messages)
+        duplicate_candiates = messages.select {|l| l.include?("ios: ")}
+        duplicated = duplicate_candiates.select {|l| messages.include?(l.gsub(/ios: /,'osx: ')) }
+        duplicated.uniq.each do |l|
+          clean = l.gsub(/ios: /,'')
+          messages.insert(messages.index(l), clean)
+          messages.delete(l)
+          messages.delete('osx: ' + clean)
+        end
+        messages
+      end
+
+      def podspecs_to_lint
+        @podspecs_to_lint ||= begin
+          if (is_repo?)
+            files = (config.repos_dir + @repo_or_podspec).glob('**/*.podspec')
+          elsif @repo_or_podspec
             files = [Pathname.new(@repo_or_podspec)]
-            raise Informative, "[!] Unable to find a spec named #{@repo_or_podspec}".red unless files[0].exist? && @repo_or_podspec.include?('.podspec')
+            raise Informative, "[!] Unable to find a spec named #{@repo_or_podspec}".red << "\n\n" unless files[0].exist? && @repo_or_podspec.include?('.podspec')
           else
             files = Pathname.pwd.glob('*.podspec')
-            raise Informative, "[!] No specs found in the current directory".red if files.empty?
+            raise Informative, "[!] No specs found in the current directory".red << "\n\n" if files.empty?
           end
+          files
         end
-        puts
-        all_valid = Linter.new(files, is_repo).lint!
-        if all_valid
-          puts (files.count == 1 ? "#{files[0].basename} passed validation" : "All the specs passed validation").green << "\n\n"
-        else
-          message = (files.count == 1 ?  "[!] The spec did not pass validation" : "[!] Not all specs passed validation").red
-        raise Informative, message
-        end
       end
 
-      def repo_with_name_exist(name)
-        name && (config.repos_dir + name).exist? && !name.include?('/')
+      def is_repo?
+        @is_repo ||= @repo_or_podspec && (config.repos_dir + @repo_or_podspec).exist? && !@repo_or_podspec.include?('/')
+      end
+
+      def lint_passed_message
+        ( podspecs_to_lint.count == 1 ? "#{podspecs_to_lint.first.basename} passed validation" : "All the specs passed validation" ).green << "\n\n"
       end
 
+      def lint_failed_message
+        ( podspecs_to_lint.count == 1 ? "[!] The spec did not pass validation" : "[!] Not all specs passed validation" ).red  << "\n\n"
+      end
+
+      # Linter class
+      #
       class Linter
         include Config::Mixin
 
-        def initialize(files, is_repo)
-          @files = files
-          @is_repo = is_repo
+        attr_reader :spec, :file, :is_repo, :quick, :only_errors
+
+        def initialize(spec, podspec_file, is_repo, quick, only_errors)
+          @spec        = spec
+          @file        = podspec_file.realpath
+          @is_repo     = is_repo
+          @quick       = quick
+          @only_errors = only_errors
         end
 
         # Takes an array of podspec files and lints them all
         #
-        # It returns true if **all** the files passed validation
+        # It returns true if the spec passed validation
         #
-        def lint!
-          all_valid = true
-          @files.each do |file|
-            file = file.realpath
-            file_spec = Specification.from_file(file)
-
-            specs = file_spec.recursive_subspecs.any? ?  file_spec.recursive_subspecs : [file_spec]
-            specs.each do |spec|
-              # Show immediatly which pod is being processed.
-              # This line will be overwritten once the result is known
-              print " -> #{spec}\r" unless config.silent? || @is_repo
-              $stdout.flush
-
-              # If the spec doesn't validate it raises and informative
-              spec.validate!
-
-              warnings     = warnings_for_spec(spec, file)
-              deprecations = deprecation_notices_for_spec(spec, file)
-              build_messages, file_patterns_errors = [], []
-              unless @is_repo || @quick
-                platform_names(spec).each do |platform_name|
-                  set_up_lint_environment
-                  build_messages       += build_errors_for_spec(spec, file, platform_name)
-                  file_patterns_errors += file_patterns_errors_for_spec(spec, file, platform_name)
-                  tear_down_lint_environment
-                end
-              end
-              build_errors   = build_messages.select {|msg| msg.include?('error')}
-              build_warnings = build_messages - build_errors
-
-              # Errors compromise the functionality of a spec, warnings can be ignored
-              all            = warnings + deprecations + build_messages + file_patterns_errors
-              errors         = file_patterns_errors + build_errors
-              warnings       = all - errors
-
-              if @only_errors
-                all_valid = false unless errors.empty?
-              else
-                # avoid to fail validation for xcode warnings
-                all_valid = false unless (all - build_warnings).empty?
-              end
-
-              clean_duplicate_platfrom_messages(errors)
-              clean_duplicate_platfrom_messages(warnings)
-
-              # This overwrites the previously printed text
-              unless config.silent?
-                if errors.empty? && warnings.empty?
-                  puts " -> ".green + "#{spec} passed validation" unless @is_repo
-                elsif errors.empty?
-                  puts " -> ".yellow + spec.to_s
-                else
-                  puts " -> ".red + spec.to_s
-                end
-              end
-
-              warnings.each {|msg| puts "    - WARN  | #{msg}"} unless config.silent?
-              errors.each   {|msg| puts "    - ERROR | #{msg}"} unless config.silent?
-              puts unless config.silent? || ( @is_repo && all.flatten.empty? )
-            end
+        def lint
+          # If the spec doesn't validate it raises and informative
+          # TODO: consider raising the informative in the clients of Pod::Specification#validate!
+          # and just report the errors here
+          spec.validate!
+          peform_multiplatform_analysis unless is_repo || quick
+          valid?
+        end
+
+        def valid?
+          only_errors ? errors.empty? : errors.empty? && warnings.empty?
+        end
+
+        def errors
+          @errors ||= file_patterns_errors + build_errors
+        end
+
+        def warnings
+          @warnings ||= podspec_warnings + deprecation_warnings
+        end
+
+        def notes
+          @notes ||= build_warnings
+        end
+
+        # Performs platform specific analysis.
+        # It requires to download the source at each iteration
+        #
+        def peform_multiplatform_analysis
+          platform_names.each do |platform_name|
+            set_up_lint_environment
+            xcodebuild_output.concat(xcodebuild_output_for_platfrom(platform_name))
+            file_patterns_errors.concat(file_patterns_errors_for_platfrom(platform_name))
+            tear_down_lint_environment
           end
-          all_valid
         end
 
-        def tmp_dir
-          Pathname.new('/tmp/CocoaPods/Lint')
+        def platform_names
+          spec.platform.name ? [spec.platform.name] : [:ios, :osx]
         end
 
         def set_up_lint_environment
@@ -178,68 +234,82 @@ module Pod
         end
 
         def tear_down_lint_environment
-          tmp_dir.rmtree
+          # tmp_dir.rmtree
           Config.instance = @original_config
         end
 
-        def clean_duplicate_platfrom_messages(messages)
-          duplicate_candiates = messages.select {|l| l.include?("ios: ")}
-          duplicated = duplicate_candiates.select {|l| messages.include?(l.gsub(/ios: /,'osx: ')) }
-          duplicated.uniq.each do |l|
-            clean = l.gsub(/ios: /,'')
-            messages.insert(messages.index(l), clean)
-            messages.delete(l)
-            messages.delete('osx: ' + clean)
-          end
+        def tmp_dir
+          Pathname.new('/tmp/CocoaPods/Lint')
+        end
+
+        def pod_dir
+          tmp_dir + 'Pods' + spec.name
         end
 
         # It checks a spec for minor non fatal defects
         #
-        # It returns a array of messages
+        # It returns a array of strings
         #
-        def warnings_for_spec(spec, file)
-          license  = spec.license
-          source   = spec.source
-          text     = file.read
+        def podspec_warnings
+          license  = @spec.license || {}
+          source   = @spec.source
+          text     = @file.read
           warnings = []
-          warnings << "The name of the spec should match the name of the file" unless path_matches_name?(file, spec)
-          warnings << "Missing license[:type]" unless license && license[:type]
-          warnings << "Github repositories should end in `.git'" if source && source[:git] =~ /github.com/ && source[:git] !~ /.*\.git/
-          warnings << "Github repositories should start with `https'" if source && source[:git] =~ /github.com/ && source[:git] !~ /https:\/\/github.com/
-          warnings << "The description should end with a dot" if spec.description != spec.summary && spec.description !~ /.*\./
-          warnings << "The summary should end with a dot" if spec.summary !~ /.*\./
-          warnings << "Missing license[:file] or [:text]" unless license && (license[:file] || license[:text])
-          warnings << "Comments must be deleted" if text =~ /^\w*#\n\w*#/ # allow a single line comment as it is generally used in subspecs
-            warnings
+          warnings << "The name of the spec should match the name of the file"  unless names_match?
+          warnings << "Missing license[:type]"                                  unless license[:type]
+          warnings << "Missing license[:file] or [:text]"                       unless license[:file] || license[:text]
+          warnings << "The summary should end with a dot"                       if @spec.summary !~ /.*\./
+          warnings << "The description should end with a dot"                   if @spec.description !~ /.*\./ && @spec.description != @spec.summary
+          warnings << "Github repositories should end in `.git'"                if github_source? && source[:git] !~ /.*\.git/
+          warnings << "Github repositories should start with `https'"           if github_source? && source[:git] !~ /https:\/\/github.com/
+          warnings << "Comments must be deleted"                                if text =~ /^\w*#\n\w*#/ # allow a single line comment as it is generally used in subspecs
+          warnings
         end
 
-        def path_matches_name?(file, spec)
-          spec_name = spec.name.match(/[^\/]*/)[0]
-          file.basename.to_s == spec_name + '.podspec'
+        def names_match?
+          root_name = @spec.name.match(/[^\/]*/)[0]
+          @file.basename.to_s == root_name + '.podspec'
+        end
+
+        def github_source?
+          @spec.source && @spec.source[:git] =~ /github.com/
         end
 
         # It reads a podspec file and checks for strings corresponding
         # to a feature that are or will be deprecated
         #
-        # It returns a array of messages
+        # It returns a array of strings
         #
-        def deprecation_notices_for_spec(spec, file)
-          text = file.read
+        def deprecation_warnings
+          text = @file.read
           deprecations = []
           deprecations << "`config.ios?' and `config.osx' will be removed in version 0.7" if text. =~ /config\..os?/
           deprecations << "The `post_install' hook is reserved for edge cases" if text. =~ /post_install/
           deprecations
         end
 
+
+        def build_errors
+          @build_errors ||= xcodebuild_output.select {|msg| msg.include?('error')}
+        end
+
+        def build_warnings
+          @build_warnings ||= xcodebuild_output - build_errors
+        end
+
+        def xcodebuild_output
+          @xcodebuild_output ||= []
+        end
+
         # It creates a podfile in memory and builds a library containing
         # the pod for all available platfroms with xcodebuild.
         #
-        # It returns a array of messages
+        # It returns a array of strings
         #
-        def build_errors_for_spec(spec, file, platform_name)
+        def xcodebuild_output_for_platfrom(platform_name)
           messages = []
           puts "\n\n#{spec} - generating build errors for #{platform_name} platform".yellow.reversed if config.verbose?
-          podfile = podfile_from_spec(spec, file, platform_name)
+          podfile = podfile_from_spec(platform_name)
           Installer.new(podfile).install!
 
           return messages if `which xcodebuild`.strip.empty?
@@ -250,10 +320,12 @@ module Pod
           messages
         end
 
-        def podfile_from_spec(spec, file, platform_name)
+        def podfile_from_spec(platform_name)
+          name    = spec.name
+          podspec = file.realpath.to_s
           podfile = Pod::Podfile.new do
             platform platform_name
-            dependency spec.name, :podspec => file.realpath.to_s
+            dependency name, :podspec => podspec
           end
         end
 
@@ -268,40 +340,41 @@ module Pod
           selected_lines.map {|l| l.gsub(/\/tmp\/CocoaPods\/Lint\/Pods\//,'')}
         end
 
+        def file_patterns_errors
+          @file_patterns_errors ||= []
+        end
+
         # It checks that every file pattern specified in a spec yields
         # at least one file. It requires the pods to be alredy present
         # in the current working directory under Pods/spec.name
         #
         # It returns a array of messages
         #
-        def file_patterns_errors_for_spec(spec, file, platform_name)
+        def file_patterns_errors_for_platfrom(platform_name)
           Dir.chdir(config.project_pods_root + spec.name ) do
             messages = []
-            messages += check_spec_files_exists(spec, :source_files, platform_name, '*.{h,m,mm,c,cpp}')
-            messages += check_spec_files_exists(spec, :resources, platform_name)
-            messages << "license[:file] = '#{spec.license[:file]}' -> did not match any file" if spec.license[:file] && Pathname.pwd.glob(spec.license[:file]).empty?
+            messages += check_spec_files_exists(:source_files, platform_name, '*.{h,m,mm,c,cpp}')
+            messages += check_spec_files_exists(:resources, platform_name)
+            messages << "#{platform_name}: license[:file] = '#{spec.license[:file]}' -> did not match any file" if spec.license[:file] && pod_dir.glob(spec.license[:file]).empty?
             messages.compact
           end
         end
 
         # TODO: FileList doesn't work and always adds the error message
         # pod spec lint ~/.cocoapods/master/MKNetworkKit/0.83/MKNetworkKit.podspec
-        def check_spec_files_exists(spec, accessor, platform_name, options = {})
+        def check_spec_files_exists(accessor, platform_name, options = {})
           result = []
           patterns = spec.send(accessor)[platform_name]
-          patterns.map do |pattern|
-            pattern = Pathname.pwd + pattern
+          patterns.map do |original_pattern|
+            pattern = pod_dir + original_pattern
             if pattern.directory? && options[:glob]
               pattern += options[:glob]
             end
-            result << "#{platform_name}: [#{accessor} = '#{pattern}'] -> did not match any file" if pattern.glob.empty?
+            result << "#{platform_name}: [#{accessor} = '#{original_pattern}'] -> did not match any file" if pattern.glob.empty?
           end
           result
         end
 
-        def platform_names(spec)
-          spec.platform.name ? [spec.platform.name] : [:ios, :osx]
-        end
       end
 
       # Templates and github information retrival for spec create