Cleanup/refactor

Problem
--------

Running `rubocop` indentified a lot of little issues for cleanup.
In doing this cleanup, there was some refactoring done that changes the
overall API.  Deprecation warnings have been added.

Solution
--------

* Add `Commands` class to pull some command running helpers out of the
  main module
* Don't mix in ImageMagick::Fonts (which removes `MojoMagick.get_fonts`
  method)
* Add deprecation warnings for missing methods
* rubocop cleanup
* make test methods shorter and better named
* Move from `Parser` to `FontParser` and add deprecations
* Sequester deprecated methods for easy removal later
* Add rubocop check to circle
* Update readme to reflect moves

Author: Jon Rogers

.circleci/config.yml

@@ -22,3 +22,4 @@ jobs:
       - run:
           name: audit
           command: bundle exec bundle-audit update && bundle exec bundle-audit check
+      - ruby/rubocop-check

.rubocop.yml

@@ -5,6 +5,7 @@ inherit_from:
   - ./.rubocop-common.yml
   - ./.rubocop-performance.yml
 
-Metrics/MethodLength:
-  Exclude:
-    - "**/*test.rb"
+Metrics/CyclomaticComplexity:
+  Max: 10
+Metrics/PerceivedComplexity:
+  Max: 10

Gemfile

@@ -1,8 +1,8 @@
 source "https://rubygems.org"
 
 group :test, :development do
-  gem 'irb', require: false
-  gem 'simplecov', require: false
+  gem "irb", require: false
+  gem "simplecov", require: false
 end
 
 gemspec

README.md

@@ -139,7 +139,7 @@ complex commands.
 
 ### Get a list of available fonts
 
-    fonts = MojoMagick::get_fonts
+    fonts = MojoMagick::available_fonts
 
     fonts.first
     => #<MojoMagick::Font:0x000001015a8b90 @name="AvantGarde-Book", @family="AvantGarde", @style="Normal", @stretch="Normal", @weight="400", @glyphs="/usr/local/share/ghostscript/fonts/a010013l.pfb">

lib/image_magick/fonts.rb

@@ -1,15 +1,8 @@
 module ImageMagick
-  module Fonts
-    def get_fonts
-      @parser ||= MojoMagick::Util::Parser.new
-      raw_fonts = begin
-        raw_command("identify", "-list font")
-      rescue Exception => e
-        puts e
-        puts "Failed to execute font list with raw_command - trying straight up execute"
-        `convert -list font`
-      end
-      @parser.parse_fonts(raw_fonts)
+  class Fonts
+    def self.all
+      raw_fonts = MojoMagick::Commands.raw_command("identify", "-list", "font")
+      MojoMagick::Util::FontParser.new(raw_fonts).parse
     end
   end
 end

lib/initializers/hash.rb

@@ -1,14 +1,12 @@
-cwd = File.dirname(__FILE__)
-require "open3"
-initializers_dir = File.expand_path(File.join(cwd, "initializers"))
-Dir.glob(File.join(initializers_dir, "*.rb")).sort.each { |f| require f }
-require File.join(cwd, "mojo_magick/util/parser")
-require File.join(cwd, "mojo_magick/errors")
-require File.join(cwd, "mojo_magick/command_status")
-require File.join(cwd, "image_magick/fonts")
-require File.join(cwd, "mojo_magick/opt_builder")
-require File.join(cwd, "mojo_magick/font")
 require "tempfile"
+require "open3"
+require_relative "./mojo_magick/util/font_parser"
+require_relative "./mojo_magick/errors"
+require_relative "./mojo_magick/command_status"
+require_relative "./mojo_magick/commands"
+require_relative "./image_magick/fonts"
+require_relative "./mojo_magick/opt_builder"
+require_relative "./mojo_magick/font"
 
 # MojoMagick is a stateless set of module methods which present a convient interface
 # for accessing common tasks for ImageMagick command line library.
@@ -34,7 +32,7 @@
 #
 # Equivalent to:
 #
-#   MojoMagick::raw_command('convert', 'source.jpg -crop 250x250+0+0\
+#   MojoMagick::Commands.raw_command('convert', 'source.jpg -crop 250x250+0+0\
 #         +repage -strip -set comment "my favorite file" dest.jpg')
 #
 # Example #mogrify usage:
@@ -43,7 +41,7 @@
 #
 # Equivalent to:
 #
-#   MojoMagick::raw_command('mogrify', '-shave 10x10 image.jpg')
+#   MojoMagick::Commands.raw_command('mogrify', '-shave 10x10 image.jpg')
 #
 # Example showing some additional options:
 #
@@ -60,34 +58,40 @@
 # bang (!) can be appended to command names to use the '+' versions
 # instead of '-' versions.
 #
-module MojoMagick
-  extend ImageMagick::Fonts
 
-  def self.windows?
-    !(RUBY_PLATFORM =~ /win32/).nil?
+module MojoMagickDeprecations
+  # rubocop:disable Naming/AccessorMethodName
+  def get_fonts
+    warn "DEPRECATION WARNING: #{__method__} is deprecated and will be removed with the next minor version release."\
+         "  Please use `available_fonts` instead"
+    MojoMagick.available_fonts
   end
 
-  def self.execute(command, *args)
-    execute = "#{command} #{args}"
-    out, outerr, status = Open3.capture3(command, *args.map(&:to_s))
-    CommandStatus.new execute, out, outerr, status
-  rescue Exception => e
-    raise MojoError, "#{e.class}: #{e.message}"
+  # rubocop:enable Naming/AccessorMethodName
+  ### Moved to `Commands`
+  def execute!(*args)
+    warn "DEPRECATION WARNING: #{__method__} is deprecated and will be removed with the next minor version release."\
+         "  Please use `MojoMagick::Commands.execute!` instead"
+    MojoMagick::Commands.send(:execute!, *args)
   end
 
-  def self.execute!(command, *args)
-    status = execute(command, *args)
-    unless status.success?
-      err_msg = "MojoMagick command failed: #{command}."
-      raise(MojoFailed, "#{err_msg} (Exit status: #{status.exit_code})\n" +
-            "  Command: #{status.command}\n" +
-            "  Error: #{status.error}")
-    end
-    status.return_value
+  def execute(*args)
+    warn "DEPRECATION WARNING: #{__method__} is deprecated and will be removed with the next minor version release."\
+         "  Please use `MojoMagick::Commands.execute!` instead"
+    MojoMagick::Commands.send(:execute, *args)
+  end
+
+  def raw_command(*args)
+    warn "DEPRECATION WARNING: #{__method__} is deprecated and will be removed with the next minor version release."\
+         "  Please use `MojoMagick::Commands.execute!` instead"
+    MojoMagick::Commands.raw_command(*args)
   end
+end
 
-  def self.raw_command(*args)
-    execute!(*args)
+module MojoMagick
+  extend MojoMagickDeprecations
+  def self.windows?
+    !RUBY_PLATFORM.include(win32)
   end
 
   def self.shrink(source_file, dest_file, options)
@@ -105,44 +109,49 @@ def self.expand(source_file, dest_file, options)
   # resizes an image and returns the filename written to
   # options:
   #   :width / :height => scale to these dimensions
-  #   :scale => pass scale options such as ">" to force shrink scaling only or "!" to force absolute width/height scaling (do not preserve aspect ratio)
+  #   :scale => pass scale options such as ">" to force shrink scaling only or
+  #             "!" to force absolute width/height scaling (do not preserve aspect ratio)
   #   :percent => scale image to this percentage (do not specify :width/:height in this case)
   def self.resize(source_file, dest_file, options)
-    scale_options = []
-    scale_options << ">" unless options[:shrink_only].nil?
-    scale_options << "<" unless options[:expand_only].nil?
-    scale_options << "!" unless options[:absolute_aspect].nil?
-    scale_options << "^" unless options[:fill].nil?
-    scale_options = scale_options.join
-
+    scale_options = extract_scale_options(options)
+    geometry = extract_geometry_options(options)
     extras = []
-    if !options[:width].nil? && !options[:height].nil?
-      geometry = "#{options[:width]}X#{options[:height]}"
-    elsif !options[:percent].nil?
-      geometry = "#{options[:percent]}%"
-    else
-      raise MojoMagickError, "Unknown options for method resize: #{options.inspect}"
-    end
     if !options[:fill].nil? && !options[:crop].nil?
       extras << "-gravity"
       extras << "Center"
       extras << "-extent"
       extras << geometry.to_s
     end
-    raw_command("convert",
-                source_file,
-                "-resize", "#{geometry}#{scale_options}",
-                *extras, dest_file)
+    Commands.raw_command("convert",
+                         source_file,
+                         "-resize", "#{geometry}#{scale_options}",
+                         *extras, dest_file)
     dest_file
   end
 
+  def self.convert(source = nil, dest = nil)
+    opts = OptBuilder.new
+    opts.file source if source
+    yield opts
+    opts.file dest if dest
+
+    Commands.raw_command("convert", *opts.to_a)
+  end
+
+  def self.mogrify(dest = nil)
+    opts = OptBuilder.new
+    yield opts
+    opts.file dest if dest
+    Commands.raw_command("mogrify", *opts.to_a)
+  end
+
   def self.available_fonts
     # returns width, height of image if available, nil if not
     Font.all
   end
 
   def self.get_format(source_file, format_string)
-    raw_command("identify", "-format", format_string, source_file)
+    Commands.raw_command("identify", "-format", format_string, source_file)
   end
 
   # returns an empty hash or a hash with :width and :height set (e.g. {:width => INT, :height => INT})
@@ -161,22 +170,6 @@ def self.get_image_size(source_file)
     { width: width, height: height }
   end
 
-  def self.convert(source = nil, dest = nil)
-    opts = OptBuilder.new
-    opts.file source if source
-    yield opts
-    opts.file dest if dest
-
-    raw_command("convert", *opts.to_a)
-  end
-
-  def self.mogrify(dest = nil)
-    opts = OptBuilder.new
-    yield opts
-    opts.file dest if dest
-    raw_command("mogrify", *opts.to_a)
-  end
-
   def self.tempfile(*opts)
     data = opts[0]
     rest = opts[1]
@@ -188,4 +181,27 @@ def self.tempfile(*opts)
   ensure
     file.close
   end
+
+  class << self
+    private
+
+    def extract_geometry_options(options)
+      if !options[:width].nil? && !options[:height].nil?
+        "#{options[:width]}X#{options[:height]}"
+      elsif !options[:percent].nil?
+        "#{options[:percent]}%"
+      else
+        raise MojoMagickError, "Resize requires width and height or percentage: #{options.inspect}"
+      end
+    end
+
+    def extract_scale_options(options)
+      [].tap { |scale_options|
+        scale_options << ">" unless options[:shrink_only].nil?
+        scale_options << "<" unless options[:expand_only].nil?
+        scale_options << "!" unless options[:absolute_aspect].nil?
+        scale_options << "^" unless options[:fill].nil?
+      }.join
+    end
+  end
 end

lib/mojo_magick.rb

@@ -0,0 +1,32 @@
+require_relative "opt_builder"
+
+module MojoMagick
+  class Commands
+    def self.raw_command(*args)
+      execute!(*args)
+    end
+
+    class << self
+      private
+
+      def execute(command, *args)
+        execute = "#{command} #{args}"
+        out, outerr, status = Open3.capture3(command, *args.map(&:to_s))
+        CommandStatus.new execute, out, outerr, status
+      rescue StandardError => e
+        raise MojoError, "#{e.class}: #{e.message}"
+      end
+
+      def execute!(command, *args)
+        status = execute(command, *args)
+        unless status.success?
+          err_msg = "MojoMagick command failed: #{command}."
+          raise(MojoFailed, "#{err_msg} (Exit status: #{status.exit_code})\n" \
+                            "  Command: #{status.command}\n" \
+                            "  Error: #{status.error}")
+        end
+        status.return_value
+      end
+    end
+  end
+end

lib/mojo_magick/commands.rb

@@ -7,15 +7,14 @@ def valid?
     end
 
     def initialize(property_hash = {})
-      property_hash.symbolize_keys!
       %i[name family style stretch weight glyphs].each do |f|
         setter = "#{f}="
         send(setter, property_hash[f])
       end
     end
 
     def self.all
-      ImageMagick::Font.all.map { |font_info| Font.new(font_info) }
+      ImageMagick::Fonts.all
     end
   end
 end

lib/mojo_magick/font.rb

@@ -58,6 +58,7 @@ def image_block(&block)
     end
 
     # Generic commands. Arguments will be formatted if necessary
+    # rubocop:disable Lint/MissingSuper, Style/MissingRespondToMissing
     def method_missing(command, *args)
       @opts << if command.to_s[-1, 1] == "!"
                  "+#{command.to_s.chop}"
@@ -68,10 +69,7 @@ def method_missing(command, *args)
       self
     end
 
-    def to_s
-      to_a.join " "
-    end
-
+    # rubocop:enable Lint/MissingSuper, Style/MissingRespondToMissing
     def to_a
       @opts.flatten
     end