Fix all Ruby warnings generated by state_machine. Closes pluginaweek#167

CHANGELOG.md

@@ -1,5 +1,6 @@
 # master
 
+* Fix all Ruby warnings
 * Fix callbacks not working for methods that respond via method_missing [Balwant Kane]
 * Fix observer callbacks being run when disabled in ActiveModel / ActiveRecord integrations
 * Add YARD integration for autogenerating documentation / embedding visualizations of state machines

Rakefile

@@ -19,6 +19,7 @@ Rake::TestTask.new(:test) do |t|
   t.libs << 'lib'
   t.test_files = integration ? Dir["test/unit/integrations/#{integration}_test.rb"] : Dir['test/{functional,unit}/*_test.rb'] + ['test/unit/integrations/base_test.rb']
   t.verbose = true
+  t.warning = true if ENV['WARNINGS']
 end
 
 namespace :appraisal do

lib/state_machine/callback.rb

@@ -175,9 +175,9 @@ def call(object, context = {}, *args, &block)
       # method passes the terminator.
       def run_methods(object, context = {}, index = 0, *args, &block)
         if type == :around
-          if method = @methods[index]
+          if current_method = @methods[index]
             yielded = false
-            evaluate_method(object, method, *args) do
+            evaluate_method(object, current_method, *args) do
               yielded = true
               run_methods(object, context, index + 1, *args, &block)
             end

lib/state_machine/eval_helpers.rb

@@ -76,12 +76,12 @@ def evaluate_method(object, method, *args, &block)
             args = args[0, arity] if [0, 1].include?(arity)
           end
 
-          method.call(*args, &block)
+          method.is_a?(Proc) ? method.call(*args) : method.call(*args, &block)
         when String
           eval(method, object.instance_eval {binding}, &block)
         else
           raise ArgumentError, 'Methods must be a symbol denoting the method to call, a block to be invoked, or a string to be evaluated'
-        end
+      end
     end
   end
 end

lib/state_machine/event.rb

@@ -58,7 +58,7 @@ def initialize(machine, name, options = {}) #:nodoc:
       reset
 
       # Output a warning if another event has a conflicting qualified name
-      if conflict = machine.owner_class.state_machines.detect {|name, other_machine| other_machine != @machine && other_machine.events[qualified_name, :qualified_name]}
+      if conflict = machine.owner_class.state_machines.detect {|other_name, other_machine| other_machine != @machine && other_machine.events[qualified_name, :qualified_name]}
         name, other_machine = conflict
         warn "Event #{qualified_name.inspect} for #{machine.name.inspect} is already defined in #{other_machine.name.inspect}"
       else

lib/state_machine/integrations/active_model.rb

@@ -376,9 +376,9 @@ def self.matching_ancestors
       def invalidate(object, attribute, message, values = [])
         if supports_validations?
           attribute = self.attribute(attribute)
-          options = values.inject({}) do |options, (key, value)|
-            options[key] = value
-            options
+          options = values.inject({}) do |h, (key, value)|
+            h[key] = value
+            h
           end
 
           default_options = default_error_message_options(object, attribute, message)
@@ -531,15 +531,15 @@ def add_callback(type, options, &block)
 
         # Configures new states with the built-in humanize scheme
         def add_states(new_states)
-          super.each do |state|
-            state.human_name = lambda {|state, klass| translate(klass, :state, state.name)}
+          super.each do |new_state|
+            new_state.human_name = lambda {|state, klass| translate(klass, :state, state.name)}
           end
         end
 
         # Configures new event with the built-in humanize scheme
         def add_events(new_events)
-          super.each do |event|
-            event.human_name = lambda {|event, klass| translate(klass, :event, event.name)}
+          super.each do |new_event|
+            new_event.human_name = lambda {|event, klass| translate(klass, :event, event.name)}
           end
         end
 

lib/state_machine/integrations/mongo_mapper/versions.rb

@@ -82,7 +82,7 @@ def self.active?
         def define_state_initializer
           define_helper :instance, <<-end_eval, __FILE__, __LINE__ + 1
             def initialize(*args)
-              attrs, from_db = args
+              from_db = args[1]
               from_db ? super : self.class.state_machines.initialize_states(self) { super }
             end
           end_eval

lib/state_machine/machine.rb

@@ -489,7 +489,7 @@ class << self; attr_accessor :default_messages; end
     @default_messages = {
       :invalid => 'is invalid',
       :invalid_event => 'cannot transition when %s',
-      :invalid_transition => 'cannot transition via "%s"'
+      :invalid_transition => 'cannot transition via "%1$s"'
     }
 
     # Whether to ignore any conflicts that are detected for helper methods that
@@ -498,7 +498,7 @@ class << self; attr_accessor :ignore_method_conflicts; end
     @ignore_method_conflicts = false
 
     # The class that the machine is defined in
-    attr_accessor :owner_class
+    attr_reader :owner_class
 
     # The name of the machine, used for scoping methods generated for the
     # machine as a whole (not states or events)
@@ -542,7 +542,7 @@ def initialize(owner_class, *args, &block)
 
       # Find an integration that matches this machine's owner class
       if options.include?(:integration)
-        @integration = StateMachine::Integrations.find_by_name(options[:integration]) if options[:integration]
+        @integration = options[:integration] && StateMachine::Integrations.find_by_name(options[:integration])
       else
         @integration = StateMachine::Integrations.match(owner_class)
       end
@@ -566,6 +566,7 @@ def initialize(owner_class, *args, &block)
       @action = options[:action]
       @use_transactions = options[:use_transactions]
       @initialize_state = options[:initialize]
+      @action_hook_defined = false
       self.owner_class = owner_class
       self.initial_state = options[:initial] unless sibling_machines.any?
 
@@ -673,7 +674,7 @@ def initial_state(object)
 
     # Whether a dynamic initial state is being used in the machine
     def dynamic_initial_state?
-      @initial_state.is_a?(Proc)
+      instance_variable_defined?('@initial_state') && @initial_state.is_a?(Proc)
     end
 
     # Initializes the state on the given object.  Initial values are only set if
@@ -749,8 +750,8 @@ def define_helper(scope, method, *args, &block)
         else
           name = self.name
           helper_module.class_eval do
-            define_method(method) do |*args|
-              block.call((scope == :instance ? self.class : self).state_machine(name), self, *args)
+            define_method(method) do |*block_args|
+              block.call((scope == :instance ? self.class : self).state_machine(name), self, *block_args)
             end
           end
         end
@@ -1074,7 +1075,11 @@ def state(*names, &block)
     #   Vehicle.state_machine.read(vehicle, :event)     # => nil      # Equivalent to vehicle.state_event
     def read(object, attribute, ivar = false)
       attribute = self.attribute(attribute)
-      ivar ? object.instance_variable_get("@#{attribute}") : object.send(attribute)
+      if ivar
+        object.instance_variable_defined?("@#{attribute}") ? object.instance_variable_get("@#{attribute}") : nil
+      else
+        object.send(attribute)
+      end
     end
 
     # Sets a new value in the given object's attribute.
@@ -1865,7 +1870,15 @@ def reset(object)
     # Generates the message to use when invalidating the given object after
     # failing to transition on a specific event
     def generate_message(name, values = [])
-      (@messages[name] || self.class.default_messages[name]) % values.map {|value| value.last}
+      message = (@messages[name] || self.class.default_messages[name])
+
+      # Check whether there are actually any values to interpolate to avoid
+      # any warnings
+      if message.scan(/%./).any? {|match| match != '%%'}
+        message % values.map {|value| value.last}
+      else
+        message
+      end
     end
 
     # Runs a transaction, rolling back any changes if the yielded block fails.

lib/state_machine/machine_collection.rb

@@ -55,7 +55,6 @@ def fire_events(object, *events)
 
         # Get the transition that will be performed for the event
         unless transition = event.transition_for(object)
-          machine = event.machine
           event.on_failure(object)
         end
 

lib/state_machine/node_collection.rb

@@ -44,7 +44,7 @@ def initialize_copy(orig) #:nodoc:
       contexts = @contexts
       @nodes = []
       @contexts = []
-      @indices = @indices.inject({}) {|indices, (name, index)| indices[name] = {}; indices}
+      @indices = @indices.inject({}) {|indices, (name, *)| indices[name] = {}; indices}
 
       # Add nodes *prior* to copying over the contexts so that they don't get
       # evaluated multiple times

lib/state_machine/state.rb

@@ -74,11 +74,11 @@ def initialize(machine, name, options = {}) #:nodoc:
       @initial = options[:initial] == true
 
       if name
-        conflicting_machines = machine.owner_class.state_machines.select {|name, other_machine| other_machine != machine && other_machine.states[qualified_name, :qualified_name]}
+        conflicting_machines = machine.owner_class.state_machines.select {|other_name, other_machine| other_machine != machine && other_machine.states[qualified_name, :qualified_name]}
 
         # Output a warning if another machine has a conflicting qualified name
         # for a different attribute
-        if conflict = conflicting_machines.detect {|name, other_machine| other_machine.attribute != machine.attribute}
+        if conflict = conflicting_machines.detect {|other_name, other_machine| other_machine.attribute != machine.attribute}
           name, other_machine = conflict
           warn "State #{qualified_name.inspect} for #{machine.name.inspect} is already defined in #{other_machine.name.inspect}"
         elsif conflicting_machines.empty?
@@ -194,6 +194,7 @@ def context(&block)
 
         # Calls the method defined by the current state of the machine
         context.class_eval <<-end_eval, __FILE__, __LINE__ + 1
+          remove_method :#{method}
           def #{method}(*args, &block)
             self.class.state_machine(#{machine_name.inspect}).states.fetch(#{name.inspect}).call(self, #{method.inspect}, lambda {super(*args, &block)}, *args, &block)
           end

lib/state_machine/state_context.rb

@@ -116,10 +116,10 @@ def method_missing(*args, &block)
 
       # Replace the configuration condition with the one configured for this
       # proxy, merging together any existing conditions
-      options[:if] = lambda do |*args|
+      options[:if] = lambda do |*condition_args|
         # Block may be executed within the context of the actual object, so
         # it'll either be the first argument or the executing context
-        object = args.first || self
+        object = condition_args.first || self
 
         proxy.evaluate_method(object, proxy_condition) &&
         Array(if_condition).all? {|condition| proxy.evaluate_method(object, condition)} &&

lib/state_machine/transition.rb

@@ -90,6 +90,7 @@ def initialize(object, machine, event, from_name, to_name, read_state = true) #:
       @machine = machine
       @args = []
       @transient = false
+      @resume_block = nil
 
       @event = machine.events.fetch(event)
       @from_state = machine.states.fetch(from_name)

lib/state_machine/transition_collection.rb

@@ -73,9 +73,10 @@ def perform(&block)
       end
     end
 
-    private
+    protected
       attr_reader :results #:nodoc:
 
+    private
       # Is this a valid set of transitions?  If the collection was creating with
       # any +false+ values for transitions, then the the collection will be
       # marked as invalid.

test/unit/branch_test.rb

@@ -707,6 +707,10 @@ def test_should_not_match_if_any_are_true
 end
 
 class BranchWithConflictingConditionalsTest < Test::Unit::TestCase
+  def setup
+    @object = Object.new
+  end
+
   def test_should_match_if_if_is_true_and_unless_is_false
     branch = StateMachine::Branch.new(:if => lambda {true}, :unless => lambda {false})
     assert branch.match(@object)

test/unit/callback_test.rb

@@ -485,7 +485,7 @@ def test_should_include_arguments_if_splat_used
 
 class CallbackWithApplicationTerminatorTest < Test::Unit::TestCase
   def setup
-    @original_terminator = StateMachine::Callback.bind_to_object
+    @original_terminator = StateMachine::Callback.terminator
     StateMachine::Callback.terminator = lambda {|result| result == false}
 
     @object = Object.new
@@ -502,7 +502,7 @@ def test_should_halt_if_terminator_matches
   end
 
   def teardown
-    StateMachine::Callback.bind_to_object = @original_bind_to_object
+    StateMachine::Callback.terminator = @original_terminator
   end
 end
 
@@ -597,6 +597,7 @@ def test_should_call_block_before_after_callbacks
 
   def test_should_halt_if_first_doesnt_yield
     class << @object
+      remove_method :run_1
       def run_1
         (@before_callbacks ||= []) << :run_1
       end
@@ -612,6 +613,7 @@ def run_1
 
   def test_should_halt_if_last_doesnt_yield
     class << @object
+      remove_method :run_2
       def run_2
         (@before_callbacks ||= []) << :run_2
       end
@@ -624,6 +626,7 @@ def run_2
 
   def test_should_not_evaluate_further_methods_if_after_halts
     class << @object
+      remove_method :run_2
       def run_2
         (@before_callbacks ||= []) << :run_2
         yield

test/unit/eval_helpers_test.rb

@@ -14,7 +14,7 @@ def setup
 
   def test_should_raise_exception_if_method_is_not_symbol_string_or_proc
     exception = assert_raise(ArgumentError) { evaluate_method(@object, 1) }
-    assert_match /Methods must/, exception.message
+    assert_match(/Methods must/, exception.message)
   end
 end
 

test/unit/event_collection_test.rb

@@ -2,8 +2,10 @@
 
 class EventCollectionByDefaultTest < Test::Unit::TestCase
   def setup
-    @machine = StateMachine::Machine.new(Class.new)
+    @klass = Class.new
+    @machine = StateMachine::Machine.new(@klass)
     @events = StateMachine::EventCollection.new(@machine)
+    @object = @klass.new
   end
 
   def test_should_not_have_any_nodes

test/unit/event_test.rb

@@ -973,6 +973,7 @@ def test_should_marshal_during_before_callbacks
 
   def test_should_marshal_during_action
     @klass.class_eval do
+      remove_method :save
       def save
         Marshal.dump(self)
       end

test/unit/integrations/active_model_test.rb

@@ -15,9 +15,9 @@ def new_model(&block)
         # Simple ActiveModel superclass
         parent = Class.new do
           def self.model_attribute(name)
-            define_method(name) { instance_variable_get("@#{name}") }
+            define_method(name) { instance_variable_defined?("@#{name}") ? instance_variable_get("@#{name}") : nil }
             define_method("#{name}=") do |value|
-              send("#{name}_will_change!") if self.class <= ActiveModel::Dirty && value != instance_variable_get("@#{name}")
+              send("#{name}_will_change!") if self.class <= ActiveModel::Dirty && value != send(name)
               instance_variable_set("@#{name}", value)
             end
           end
@@ -92,7 +92,7 @@ def test_should_not_match_if_class_does_not_include_active_model_features
     end
 
     def test_should_have_no_defaults
-      assert_equal e = {}, StateMachine::Integrations::ActiveModel.defaults
+      assert_equal({}, StateMachine::Integrations::ActiveModel.defaults)
     end
 
     def test_should_have_a_locale_path
@@ -584,7 +584,7 @@ def test_should_not_run_further_callbacks
   end
 
   class MachineWithFailedAfterCallbacksTest < BaseTestCase
-     def setup
+    def setup
       @callbacks = []
 
       @model = new_model
@@ -867,7 +867,6 @@ def test_should_support_nil_from_states
         :before_transition_state_from_nil
       ]
 
-      notified = false
       observer = new_observer(@model) do
         callbacks.each do |callback|
           define_method(callback) do |*args|
@@ -891,7 +890,6 @@ def test_should_support_nil_to_states
         :before_transition_state_to_nil
       ]
 
-      notified = false
       observer = new_observer(@model) do
         callbacks.each do |callback|
           define_method(callback) do |*args|