Merge pull request #365 from BBC-News/final_pr

Final PR by @DaveBlooman

lib/wraith/cli.rb

@@ -45,7 +45,7 @@ def copy_old_shots(config_name)
   desc "validate", "checks your configuration and validates that all required properties exist"
   def validate(config_name)
     within_acceptable_limits do
-      wraith = Wraith::Validate.new(config_name).validate
+      Wraith::Validate.new(config_name).validate
     end
   end
 
@@ -128,7 +128,7 @@ def generate_gallery(config_name, multi = false)
   desc "capture [config_name]", "Capture paths against two domains, compare them, generate gallery"
   def capture(config, multi = false)
     within_acceptable_limits do
-      logger.info Wraith::Validate.new(config).validate('capture')
+      logger.info Wraith::Validate.new(config).validate("capture")
       reset_shots(config)
       check_for_paths(config)
       setup_folders(config)
@@ -153,7 +153,7 @@ def multi_capture(filelist)
   desc "history [config_name]", "Setup a baseline set of shots"
   def history(config)
     within_acceptable_limits do
-      logger.info Wraith::Validate.new(config).validate('history')
+      logger.info Wraith::Validate.new(config).validate("history")
       reset_shots(config)
       check_for_paths(config)
       setup_folders(config)
@@ -165,7 +165,7 @@ def history(config)
   desc "latest [config_name]", "Capture new shots to compare with baseline"
   def latest(config)
     within_acceptable_limits do
-      logger.info Wraith::Validate.new(config).validate('latest')
+      logger.info Wraith::Validate.new(config).validate("latest")
       reset_shots(config)
       save_images(config, true)
       copy_base_images(config)
@@ -175,4 +175,4 @@ def latest(config)
       generate_gallery(config)
     end
   end
-end
+end

lib/wraith/folder.rb

@@ -37,7 +37,7 @@ def clear_shots_folder
 
   def copy_old_shots
     if history_dir.nil?
-      logger.error 'no `history_dir` attribute found in config. Cannot copy files.'
+      logger.error "no `history_dir` attribute found in config. Cannot copy files."
     else
       FileUtils.cp_r("#{dir}/.", "#{history_dir}/")
     end

lib/wraith/gallery.rb

@@ -52,9 +52,7 @@ def matcher(match, filename, dirname, category)
     @thumbnail = "thumbnails/#{category}/#{filename}"
     @url = figure_out_url @group, category
 
-    if @dirs[category][@size].nil?
-      @dirs[category][@size] = { :variants => [] }
-    end
+    @dirs[category][@size] = { :variants => [] } if @dirs[category][@size].nil?
 
     size_dict = @dirs[category][@size]
 
@@ -63,20 +61,20 @@ def matcher(match, filename, dirname, category)
 
   def figure_out_url(group, category)
     root = wraith.domains["#{group}"]
-    return '' if root.nil?
+    return "" if root.nil?
     path = get_path(category)
     url  = root + path
     url
   end
 
   def get_path(category)
-    wraith.paths[category]['path'] || wraith.paths[category]
+    wraith.paths[category]["path"] || wraith.paths[category]
   end
 
   def get_group_from_match(match)
     group = match[2]
-    dash = match[2].rindex('-')
-    group = match[2][dash+1..-1] unless dash.nil?
+    dash = match[2].rindex("-")
+    group = match[2][dash + 1..-1] unless dash.nil?
     group
   end
 
@@ -159,7 +157,7 @@ def check_failed_shots
 
       @dirs.each do |dir, sizes|
         sizes.to_a.sort.each do |size, files|
-          file = dir.gsub('__', '/')
+          file = dir.gsub("__", "/")
           if !files.include?(:diff)
             logger.warn "\t Unable to create a diff image for #{file}"
           elsif files[:data] > wraith.threshold
@@ -176,7 +174,7 @@ def check_failed_shots
 
   def prompt_user_to_open_gallery(dest)
     logger.info "\nView the gallery in your browser:"
-    logger.info "\t file://" + Dir.pwd + '/' + dest
+    logger.info "\t file://" + Dir.pwd + "/" + dest
   end
 
   class ErbBinding < OpenStruct

lib/wraith/helpers/capture_options.rb

@@ -10,7 +10,7 @@ def initialize(options, wraith)
   end
 
   def path
-    has_casper(options)
+    casper?(options)
   end
 
   def selector
@@ -19,15 +19,15 @@ def selector
 
   def resize
     # path level, or YAML-file level `resize_or_reload` property value
-    if @options["resize_or_reload"]
-      (@options["resize_or_reload"] == 'resize')
+    if options["resize_or_reload"]
+      (options["resize_or_reload"] == "resize")
     else
-      @wraith.resize
+      wraith.resize
     end
   end
 
   def before_capture
-    @options["before_capture"] ? convert_to_absolute(@options["before_capture"]) : false
+    options["before_capture"] ? convert_to_absolute(options["before_capture"]) : false
   end
 
   def base_url
@@ -46,7 +46,7 @@ def compare_urls(path)
     wraith.comp_domain + path unless wraith.comp_domain.nil?
   end
 
-  def has_casper(options)
+  def casper?(options)
     options["path"] ? options["path"] : options
   end
 end

lib/wraith/helpers/logger.rb

@@ -1,5 +1,5 @@
 # Logging Module, credit: http://stackoverflow.com/a/6768164
-require 'logger'
+require "logger"
 
 module Logging
   # This is the magical bit that gets mixed into your classes
@@ -11,10 +11,10 @@ def logger
   def self.logger
     unless @logger
       @logger = Logger.new(STDOUT)
-      @logger.formatter = proc do |severity, datetime, progname, msg|
-        (severity == 'INFO') ? "#{msg}\n" : "#{severity}: #{msg}\n"
+      @logger.formatter = proc do |severity, _datetime, _progname, msg|
+        (severity == "INFO") ? "#{msg}\n" : "#{severity}: #{msg}\n"
       end
     end
     @logger
   end
-end
+end

lib/wraith/helpers/utilities.rb

@@ -2,8 +2,8 @@
 
 def convert_to_absolute(filepath)
   if !filepath
-    'false'
-  elsif filepath[0] == '/'
+    "false"
+  elsif filepath[0] == "/"
     # filepath is already absolute. return unchanged
     filepath
   else

lib/wraith/save_images.rb

@@ -36,10 +36,10 @@ def define_jobs
       settings = CaptureOptions.new(options, wraith)
 
       if settings.resize
-        jobs = jobs + define_individual_job(label, settings, wraith.widths)
+        jobs += define_individual_job(label, settings, wraith.widths)
       else
         wraith.widths.each do |width|
-          jobs = jobs + define_individual_job(label, settings, width)
+          jobs += define_individual_job(label, settings, width)
         end
       end
     end
@@ -59,7 +59,7 @@ def define_individual_job(label, settings, width)
 
   def prepare_widths_for_cli(width)
     # prepare for the command line. [30,40,50] => "30,40,50"
-    width = width.join(',') if width.is_a? Array
+    width = width.join(",") if width.is_a? Array
     width
   end
 
@@ -101,10 +101,10 @@ def attempt_image_capture(capture_page_image, filename)
       run_command capture_page_image
 
       if wraith.resize
-        return # @TODO - need to check if the image was generated, as per the reload example below
+        next # @TODO - need to check if the image was generated, as per the reload example below
       end
 
-      return if File.exist? filename
+      next if File.exist? filename
 
       logger.warn "Failed to capture image #{filename} on attempt number #{i + 1} of #{max_attempts}"
     end

lib/wraith/spider.rb

@@ -6,19 +6,20 @@
 
 class Wraith::Spidering
   include Logging
+  attr_reader :wraith
 
   def initialize(config)
     @wraith = Wraith::Wraith.new(config)
   end
 
   def check_for_paths
-    if @wraith.paths.nil?
-      unless @wraith.sitemap.nil?
+    if wraith.paths.nil?
+      unless wraith.sitemap.nil?
         logger.info "no paths defined in config, loading paths from sitemap"
-        spider = Wraith::Sitemap.new(@wraith)
+        spider = Wraith::Sitemap.new(wraith)
       else
         logger.info "no paths defined in config, crawling from site root"
-        spider = Wraith::Crawler.new(@wraith)
+        spider = Wraith::Crawler.new(wraith)
       end
       spider.determine_paths
     end
@@ -39,7 +40,7 @@ def determine_paths
   private
 
   def write_file
-    File.open(@wraith.spider_file, "w+") { |file| file.write(@paths) }
+    File.open(wraith.spider_file, "w+") { |file| file.write(@paths) }
   end
 
   def add_path(path)
@@ -60,16 +61,15 @@ class Wraith::Crawler < Wraith::Spider
            m3u f4v pdf doc xls ppt pps bin exe rss xml)
 
   def spider
-    if File.exist?(@wraith.spider_file) && modified_since(@wraith.spider_file, @wraith.spider_days[0])
+    if File.exist?(wraith.spider_file) && modified_since(wraith.spider_file, wraith.spider_days[0])
       logger.info "using existing spider file"
-      @paths = eval(File.read(@wraith.spider_file))
+      @paths = eval(File.read(wraith.spider_file))
     else
       logger.info "creating new spider file"
-      spider_list = []
-      Anemone.crawl(@wraith.base_domain) do |anemone|
+      Anemone.crawl(wraith.base_domain) do |anemone|
         anemone.skip_links_like(/\.(#{EXT.join('|')})$/)
         # Add user specified skips
-        anemone.skip_links_like(@wraith.spider_skips)
+        anemone.skip_links_like(wraith.spider_skips)
         anemone.on_every_page { |page| add_path(page.url.path) }
       end
     end
@@ -84,21 +84,20 @@ class Wraith::Sitemap < Wraith::Spider
   include Logging
 
   def spider
-    unless @wraith.sitemap.nil?
-      logger.info "reading sitemap.xml from #{@wraith.sitemap}"
-      if @wraith.sitemap =~ URI.regexp
-        sitemap = Nokogiri::XML(open(@wraith.sitemap))
+    unless wraith.sitemap.nil?
+      logger.info "reading sitemap.xml from #{wraith.sitemap}"
+      if wraith.sitemap =~ URI.regexp
+        sitemap = Nokogiri::XML(open(wraith.sitemap))
       else
-        sitemap = Nokogiri::XML(File.open(@wraith.sitemap))
+        sitemap = Nokogiri::XML(File.open(wraith.sitemap))
       end
-      urls = {}
       sitemap.css("loc").each do |loc|
         path = loc.content
         # Allow use of either domain in the sitemap.xml
-        @wraith.domains.each do |_k, v|
+        wraith.domains.each do |_k, v|
           path.sub!(v, "")
         end
-        if @wraith.spider_skips.nil? || @wraith.spider_skips.none? { |regex| regex.match(path) }
+        if wraith.spider_skips.nil? || wraith.spider_skips.none? { |regex| regex.match(path) }
           add_path(path)
         end
       end

lib/wraith/validate.rb

@@ -4,27 +4,28 @@
 
 class Wraith::Validate
   include Logging
+  attr_reader :wraith
 
   def initialize(config, yaml_passed = false)
     @wraith = Wraith::Wraith.new(config, yaml_passed)
   end
 
   def validate(mode = false)
-    list_debug_information if @wraith.verbose
+    list_debug_information if wraith.verbose
     validate_basic_properties
     validate_mode_properties(mode) if mode
     # if we get this far, we've only had warnings at worst, not errors.
     "Config validated. No serious issues found."
   end
 
   def validate_basic_properties
-    if @wraith.engine.nil?
-      raise MissingRequiredPropertyError, "You must specify a browser engine! #{docs_prompt}"
+    if wraith.engine.nil?
+      fail MissingRequiredPropertyError, "You must specify a browser engine! #{docs_prompt}"
     end
-    unless @wraith.domains
-      raise MissingRequiredPropertyError, "You must specify at least one domain for Wraith to do anything! #{docs_prompt}"
+    unless wraith.domains
+      fail MissingRequiredPropertyError, "You must specify at least one domain for Wraith to do anything! #{docs_prompt}"
     end
-    #@TODO validate fuzz is not nil, etc
+    # @TODO validate fuzz is not nil, etc
   end
 
   def validate_mode_properties(mode)
@@ -42,25 +43,21 @@ def validate_mode_properties(mode)
   end
 
   def validate_capture_mode
-    if @wraith.domains.length != 2
-      raise InvalidDomainsError, "`wraith capture` requires exactly two domains. #{docs_prompt}"
-    end
-    if @wraith.history_dir
-      logger.warn "You have specified a `history_dir` in your config, but this is used in `history` mode, NOT `capture` mode. #{docs_prompt}"
-    end
+    fail InvalidDomainsError, "`wraith capture` requires exactly two domains. #{docs_prompt}" if wraith.domains.length != 2
+
+    logger.warn "You have specified a `history_dir` in your config, but this is"\
+                " used in `history` mode, NOT `capture` mode. #{docs_prompt}" if wraith.history_dir
   end
 
   def validate_history_mode
-    unless @wraith.history_dir
-      raise MissingRequiredPropertyError, "You must specify a `history_dir` to run Wraith in history mode. #{docs_prompt}"
-    end
-    if @wraith.domains.length != 1
-      raise InvalidDomainsError, "History mode requires exactly one domain. #{docs_prompt}"
-    end
+    fail MissingRequiredPropertyError, "You must specify a `history_dir` to run"\
+                  " Wraith in history mode. #{docs_prompt}" unless wraith.history_dir
+
+    fail InvalidDomainsError, "History mode requires exactly one domain. #{docs_prompt}" if wraith.domains.length != 1
   end
 
   def validate_base_shots_exist
-    unless File.directory?(@wraith.history_dir)
+    unless File.directory?(wraith.history_dir)
       logger.error "You need to run `wraith history` at least once before you can run `wraith latest`!"
     end
   end
@@ -71,10 +68,10 @@ def docs_prompt
 
   def list_debug_information
     wraith_version      = Wraith::VERSION
-    ruby_version        = run_command_safely('ruby -v')             || 'Ruby not installed'
-    phantomjs_version   = run_command_safely('phantomjs --version') || 'PhantomJS not installed'
-    casperjs_version    = run_command_safely('casperjs --version')  || 'CasperJS not installed'
-    imagemagick_version = run_command_safely('convert -version')    || 'ImageMagick not installed'
+    ruby_version        = run_command_safely("ruby -v") || "Ruby not installed"
+    phantomjs_version   = run_command_safely("phantomjs --version") || "PhantomJS not installed"
+    casperjs_version    = run_command_safely("casperjs --version") || "CasperJS not installed"
+    imagemagick_version = run_command_safely("convert -version") || "ImageMagick not installed"
 
     logger.debug "#################################################"
     logger.debug "  Wraith version:     #{wraith_version}"
@@ -90,9 +87,9 @@ def list_debug_information
   def run_command_safely(command)
     begin
       output = `#{command}`
-    rescue Exception => e
+    rescue StandardError
       return false
     end
     output.lines.first
   end
-end
+end