Merge pull request #132 from pjskennedy/remove-object-monkey-patch

Remove Object#instance_values monkey patch

20 files changed, 63 insertions, 35 deletions

diff --git a/lib/axlsx.rb b/lib/axlsx.rb
index 4e804513..f2403893 100644
--- a/lib/axlsx.rb
+++ b/lib/axlsx.rb
@@ -30,16 +30,6 @@ require 'zip'
 require 'bigdecimal'
 require 'time'
 
-#if object does not have this already, I am borrowing it from active_support.
-# I am a very big fan of activesupports instance_values method, but do not want to require nor include the entire
-# library just for this one method.
-if !Object.respond_to?(:instance_values)
-  Object.send :public  # patch for 1.8.7 as it uses private scope
-  Object.send :define_method, :instance_values do
-    Hash[instance_variables.map { |name| [name.to_s[1..-1], instance_variable_get(name)] }]
-  end
-end
-
 # xlsx generation with charts, images, automated column width, customizable styles
 # and full schema validation. Axlsx excels at helping you generate beautiful
 # Office Open XML Spreadsheet documents without having to understand the entire
@@ -47,6 +37,13 @@ end
 # Best of all, you can validate your xlsx file before serialization so you know
 # for sure that anything generated is going to load on your client's machine.
 module Axlsx
+  # I am a very big fan of activesupports instance_values method, but do not want to require nor include the entire
+  # library just for this one method.
+  #
+  # Defining as a class method on Axlsx to refrain from monkeypatching Object for all users of this gem.
+  def self.instance_values_for(object)
+    Hash[object.instance_variables.map { |name| [name.to_s[1..-1], object.instance_variable_get(name)] }]
+  end
 
   # determines the cell range for the items provided
   def self.cell_range(cells, absolute=true)
diff --git a/lib/axlsx/content_type/abstract_content_type.rb b/lib/axlsx/content_type/abstract_content_type.rb
index 832ccce6..82a659dd 100644
--- a/lib/axlsx/content_type/abstract_content_type.rb
+++ b/lib/axlsx/content_type/abstract_content_type.rb
@@ -24,7 +24,7 @@ module Axlsx
     # Serialize the contenty type to xml
     def to_xml_string(node_name = '', str = '')
       str << "<#{node_name} "
-      str << instance_values.map { |key, value| Axlsx::camel(key) << '="' << value.to_s << '"' }.join(' ')
+      str << Axlsx.instance_values_for(self).map { |key, value| Axlsx::camel(key) << '="' << value.to_s << '"' }.join(' ')
       str << '/>'
     end
 
diff --git a/lib/axlsx/doc_props/app.rb b/lib/axlsx/doc_props/app.rb
index b0dadf40..c7df1fcb 100644
--- a/lib/axlsx/doc_props/app.rb
+++ b/lib/axlsx/doc_props/app.rb
@@ -223,7 +223,7 @@ module Axlsx
     def to_xml_string(str = '')
       str << '<?xml version="1.0" encoding="UTF-8"?>'
       str << ('<Properties xmlns="' << APP_NS << '" xmlns:vt="' << APP_NS_VT << '">')
-      instance_values.each do |key, value|
+      Axlsx.instance_values_for(self).each do |key, value|
         node_name = Axlsx.camel(key)
         str << "<#{node_name}>#{value}</#{node_name}>"
       end
diff --git a/lib/axlsx/drawing/d_lbls.rb b/lib/axlsx/drawing/d_lbls.rb
index 3cb8409f..0fb3c6ad 100644
--- a/lib/axlsx/drawing/d_lbls.rb
+++ b/lib/axlsx/drawing/d_lbls.rb
@@ -71,9 +71,10 @@ module Axlsx
     def to_xml_string(str = '')
       validate_attributes_for_chart_type
       str << '<c:dLbls>'
+      instance_vals = Axlsx.instance_values_for(self)
       %w(d_lbl_pos show_legend_key show_val show_cat_name show_ser_name show_percent show_bubble_size show_leader_lines).each do |key|
-        next unless instance_values.keys.include?(key) && instance_values[key] != nil
-        str <<  "<c:#{Axlsx::camel(key, false)} val='#{instance_values[key]}' />" 
+        next unless instance_vals.keys.include?(key) && instance_vals[key] != nil
+        str <<  "<c:#{Axlsx::camel(key, false)} val='#{instance_vals[key]}' />" 
       end
       str << '</c:dLbls>'
     end
diff --git a/lib/axlsx/drawing/view_3D.rb b/lib/axlsx/drawing/view_3D.rb
index 55bbc6c3..2223d501 100644
--- a/lib/axlsx/drawing/view_3D.rb
+++ b/lib/axlsx/drawing/view_3D.rb
@@ -107,7 +107,7 @@ module Axlsx
     private
     # Note: move this to Axlsx module if we find the smae pattern elsewhere.
     def element_for_attribute(name, namespace='')
-      val = instance_values[name]
+      val = Axlsx.instance_values_for(self)[name]
       return "" if val == nil
       "<%s:%s val='%s'/>" % [namespace, Axlsx::camel(name, false), val]
     end
diff --git a/lib/axlsx/rels/relationship.rb b/lib/axlsx/rels/relationship.rb
index ad47c9a3..157f316e 100644
--- a/lib/axlsx/rels/relationship.rb
+++ b/lib/axlsx/rels/relationship.rb
@@ -103,7 +103,7 @@ module Axlsx
     # @param [String] str
     # @return [String]
     def to_xml_string(str = '')
-      h = self.instance_values.reject{|k, _| k == "source_obj"}
+      h = Axlsx.instance_values_for(self).reject{|k, _| k == "source_obj"}
       str << '<Relationship '
       str << (h.map { |key, value| '' << key.to_s << '="' << Axlsx::coder.encode(value.to_s) << '"'}.join(' '))
       str << '/>'
diff --git a/lib/axlsx/stylesheet/font.rb b/lib/axlsx/stylesheet/font.rb
index d9a4a4d8..48d2b584 100644
--- a/lib/axlsx/stylesheet/font.rb
+++ b/lib/axlsx/stylesheet/font.rb
@@ -147,7 +147,7 @@ module Axlsx
     # @return [String]
     def to_xml_string(str = '')
       str << '<font>'
-      instance_values.each do |k, v|
+      Axlsx.instance_values_for(self).each do |k, v|
         v.is_a?(Color) ? v.to_xml_string(str) : (str << ('<' << k.to_s << ' val="' << Axlsx.booleanize(v).to_s << '"/>'))
       end
       str << '</font>'
diff --git a/lib/axlsx/stylesheet/styles.rb b/lib/axlsx/stylesheet/styles.rb
index d1ee1f66..6bfd0a52 100644
--- a/lib/axlsx/stylesheet/styles.rb
+++ b/lib/axlsx/stylesheet/styles.rb
@@ -279,7 +279,7 @@ module Axlsx
     # @return [Font|Integer]
     def parse_font_options(options={})
       return if (options.keys & [:fg_color, :sz, :b, :i, :u, :strike, :outline, :shadow, :charset, :family, :font_name]).empty?
-      fonts.first.instance_values.each do |key, value|
+      Axlsx.instance_values_for(fonts.first).each do |key, value|
         # Thanks for that 1.8.7 - cant do a simple merge...
         options[key.to_sym] = value unless options.keys.include?(key.to_sym)
       end
@@ -438,8 +438,9 @@ module Axlsx
     # @return [String]
     def to_xml_string(str = '')
       str << ('<styleSheet xmlns="' << XML_NS << '">')
+      instance_vals = Axlsx.instance_values_for(self)
       [:numFmts, :fonts, :fills, :borders, :cellStyleXfs, :cellXfs, :cellStyles, :dxfs, :tableStyles].each do |key|
-        self.instance_values[key.to_s].to_xml_string(str) unless self.instance_values[key.to_s].nil?
+        instance_vals[key.to_s].to_xml_string(str) unless instance_vals[key.to_s].nil?
       end
       str << '</styleSheet>'
     end
diff --git a/lib/axlsx/util/serialized_attributes.rb b/lib/axlsx/util/serialized_attributes.rb
index d6b5d3c7..06d1697a 100644
--- a/lib/axlsx/util/serialized_attributes.rb
+++ b/lib/axlsx/util/serialized_attributes.rb
@@ -61,7 +61,7 @@ module Axlsx
     # seraialized_attributes and are not nil.
     # This requires ruby 1.9.3 or higher
     def declared_attributes
-      instance_values.select do |key, value|
+      Axlsx.instance_values_for(self).select do |key, value|
         value != nil && self.class.xml_attributes.include?(key.to_sym)
       end
     end
@@ -75,7 +75,7 @@ module Axlsx
     # @return [String] The serialized output.
     def serialized_element_attributes(str='', additional_attributes=[], &block)
       attrs = self.class.xml_element_attributes + additional_attributes
-      values = instance_values
+      values = Axlsx.instance_values_for(self)
       attrs.each do |attribute_name|
         value = values[attribute_name.to_s]
         next if value.nil?
diff --git a/lib/axlsx/workbook/worksheet/cell_serializer.rb b/lib/axlsx/workbook/worksheet/cell_serializer.rb
index eb834b08..962b575d 100644
--- a/lib/axlsx/workbook/worksheet/cell_serializer.rb
+++ b/lib/axlsx/workbook/worksheet/cell_serializer.rb
@@ -22,7 +22,7 @@ module Axlsx
       def run_xml_string(cell, str = '')
         if cell.is_text_run?
           valid = RichTextRun::INLINE_STYLES - [:value, :type]
-          data = Hash[cell.instance_values.map{ |k, v| [k.to_sym, v] }]
+          data = Hash[Axlsx.instance_values_for(cell).map{ |k, v| [k.to_sym, v] }]
           data = data.select { |key, value| valid.include?(key) && !value.nil? }
           RichText.new(cell.value.to_s, data).to_xml_string(str)
         elsif cell.contains_rich_text?
diff --git a/lib/axlsx/workbook/worksheet/data_validation.rb b/lib/axlsx/workbook/worksheet/data_validation.rb
index 1c5f5051..835b5b33 100644
--- a/lib/axlsx/workbook/worksheet/data_validation.rb
+++ b/lib/axlsx/workbook/worksheet/data_validation.rb
@@ -216,7 +216,7 @@ module Axlsx
       valid_attributes = get_valid_attributes
 
       str << '<dataValidation '
-      str << instance_values.map do |key, value|
+      str << Axlsx.instance_values_for(self).map do |key, value|
         '' << key << '="' << Axlsx.booleanize(value).to_s << '"' if (valid_attributes.include?(key.to_sym) && !CHILD_ELEMENTS.include?(key.to_sym))
       end.join(' ')
       str << '>'
diff --git a/lib/axlsx/workbook/worksheet/rich_text_run.rb b/lib/axlsx/workbook/worksheet/rich_text_run.rb
index c8668da6..b3870672 100644
--- a/lib/axlsx/workbook/worksheet/rich_text_run.rb
+++ b/lib/axlsx/workbook/worksheet/rich_text_run.rb
@@ -190,7 +190,7 @@ module Axlsx
     # @return [String]
     def to_xml_string(str = '')
       valid = RichTextRun::INLINE_STYLES
-      data = Hash[self.instance_values.map{ |k, v| [k.to_sym, v] }]
+      data = Hash[Axlsx.instance_values_for(self).map{ |k, v| [k.to_sym, v] }]
       data = data.select { |key, value| valid.include?(key) && !value.nil? }
 
       str << '<r><rPr>'
diff --git a/lib/axlsx/workbook/worksheet/worksheet.rb b/lib/axlsx/workbook/worksheet/worksheet.rb
index 0942daa2..d782fefe 100644
--- a/lib/axlsx/workbook/worksheet/worksheet.rb
+++ b/lib/axlsx/workbook/worksheet/worksheet.rb
@@ -168,7 +168,7 @@ module Axlsx
     # @return Boolean
     # @see #page_setup
     def fit_to_page?
-      return false unless self.instance_values.keys.include?('page_setup')
+      return false unless Axlsx.instance_values_for(self).keys.include?('page_setup')
       page_setup.fit_to_page?
     end
 
diff --git a/test/drawing/tc_chart.rb b/test/drawing/tc_chart.rb
index 64a5435b..302d13cd 100644
--- a/test/drawing/tc_chart.rb
+++ b/test/drawing/tc_chart.rb
@@ -103,7 +103,8 @@ class TestChart < Test::Unit::TestCase
   end
 
   def test_d_lbls
-    assert_equal(nil, @chart.instance_values[:d_lbls])
+    
+    assert_equal(nil, Axlsx.instance_values_for(@chart)[:d_lbls])
     @chart.d_lbls.d_lbl_pos = :t
     assert(@chart.d_lbls.is_a?(Axlsx::DLbls), 'DLbls instantiated on access')
   end
diff --git a/test/drawing/tc_d_lbls.rb b/test/drawing/tc_d_lbls.rb
index b8d3d1c4..dddde3a1 100644
--- a/test/drawing/tc_d_lbls.rb
+++ b/test/drawing/tc_d_lbls.rb
@@ -50,7 +50,7 @@ class TestDLbls < Test::Unit::TestCase
       @d_lbls.to_xml_string(str) 
       str << '</c:chartSpace>'
       doc = Nokogiri::XML(str)
-      @d_lbls.instance_values.each do |name, value|
+      Axlsx.instance_values_for(@d_lbls).each do |name, value|
         assert(doc.xpath("//c:#{Axlsx::camel(name, false)}[@val='#{value}']"), "#{name} is properly serialized")
       end
   end
diff --git a/test/drawing/tc_picture_locking.rb b/test/drawing/tc_picture_locking.rb
index 4c531d1a..c0a828aa 100644
--- a/test/drawing/tc_picture_locking.rb
+++ b/test/drawing/tc_picture_locking.rb
@@ -8,7 +8,7 @@ class TestPictureLocking < Test::Unit::TestCase
   end
 
   def test_initialiation
-    assert_equal(@item.instance_values.size, 1)
+    assert_equal(Axlsx.instance_values_for(@item).size, 1)
     assert_equal(@item.noChangeAspect, true)
   end
 
diff --git a/test/stylesheet/tc_styles.rb b/test/stylesheet/tc_styles.rb
index c46b6bdb..503068c0 100644
--- a/test/stylesheet/tc_styles.rb
+++ b/test/stylesheet/tc_styles.rb
@@ -136,12 +136,13 @@ class TestStyles < Test::Unit::TestCase
     original = @styles.fonts.first
     @styles.add_style :b => 1, :sz => 99
     created = @styles.fonts.last
-    original_attributes = original.instance_values
+    original_attributes = Axlsx.instance_values_for(original)
     assert_equal(1, created.b)
     assert_equal(99, created.sz)
     copied = original_attributes.reject{ |key, value| %w(b sz).include? key }
+    instance_vals = Axlsx.instance_values_for(created)
     copied.each do |key, value|
-      assert_equal(created.instance_values[key], value)
+      assert_equal(instance_vals[key], value)
     end
   end
 
diff --git a/test/tc_axlsx.rb b/test/tc_axlsx.rb
index 55bca1f4..e58ebafe 100644
--- a/test/tc_axlsx.rb
+++ b/test/tc_axlsx.rb
@@ -106,4 +106,30 @@ class TestAxlsx < Test::Unit::TestCase
     assert_equal(sanitized_str,           legit_str,            'should preserve value')
     assert_equal(sanitized_str.object_id, legit_str.object_id,  'should preserve object')
   end
+  
+  class InstanceValuesSubject
+    def initialize(args={})
+      args.each do |key, v|
+        instance_variable_set("@#{key}".to_sym, v)
+      end
+    end
+  end
+
+  def test_instance_values_for
+    empty = InstanceValuesSubject.new
+    assert_equal({}, Axlsx.instance_values_for(empty), 'should generate with no ivars')
+
+    single = InstanceValuesSubject.new(a: 2)
+    assert_equal({"a" => 2}, Axlsx.instance_values_for(single), 'should generate for a single ivar')
+
+    double = InstanceValuesSubject.new(a: 2, b: "c")
+    assert_equal({"a" => 2, "b" => "c"}, Axlsx.instance_values_for(double), 'should generate for multiple ivars')
+
+    inner_obj = Object.new
+    complex = InstanceValuesSubject.new(obj: inner_obj)
+    assert_equal({"obj" => inner_obj}, Axlsx.instance_values_for(complex), 'should pass value of ivar directly')
+
+    nil_subject = InstanceValuesSubject.new(nil_obj: nil)
+    assert_equal({"nil_obj" =>  nil}, Axlsx.instance_values_for(nil_subject), 'should return nil ivars')    
+  end
 end
diff --git a/test/tc_package.rb b/test/tc_package.rb
index b98fa2f0..b07f0d2f 100644
--- a/test/tc_package.rb
+++ b/test/tc_package.rb
@@ -97,12 +97,12 @@ class TestPackage < Test::Unit::TestCase
   end
 
   def test_core_accessor
-    assert_equal(@package.core, @package.instance_values["core"])
+    assert_equal(@package.core, Axlsx.instance_values_for(@package)["core"])
     assert_raise(NoMethodError) {@package.core = nil }
   end
 
   def test_app_accessor
-    assert_equal(@package.app, @package.instance_values["app"])
+    assert_equal(@package.app, Axlsx.instance_values_for(@package)["app"])
     assert_raise(NoMethodError) {@package.app = nil }
   end
 
@@ -114,8 +114,8 @@ class TestPackage < Test::Unit::TestCase
   end
 
   def test_default_objects_are_created
-    assert(@package.instance_values["app"].is_a?(Axlsx::App), 'App object not created')
-    assert(@package.instance_values["core"].is_a?(Axlsx::Core), 'Core object not created')
+    assert(Axlsx.instance_values_for(@package)["app"].is_a?(Axlsx::App), 'App object not created')
+    assert(Axlsx.instance_values_for(@package)["core"].is_a?(Axlsx::Core), 'Core object not created')
     assert(@package.workbook.is_a?(Axlsx::Workbook), 'Workbook object not created')
     assert(Axlsx::Package.new.workbook.worksheets.size == 0, 'Workbook should not have sheets by default')
   end
diff --git a/test/workbook/worksheet/tc_worksheet_hyperlink.rb b/test/workbook/worksheet/tc_worksheet_hyperlink.rb
index 748082ac..88a40bc6 100644
--- a/test/workbook/worksheet/tc_worksheet_hyperlink.rb
+++ b/test/workbook/worksheet/tc_worksheet_hyperlink.rb
@@ -22,7 +22,8 @@ class TestWorksheetHyperlink < Test::Unit::TestCase
   end
 
   def test_target
-    assert_equal(@options[:target], @a.instance_values['target'])
+    
+    assert_equal(@options[:target], Axlsx.instance_values_for(@a)['target'])
   end
 
   def test_display