Merge pull request #57 from rylwin/change-serialize-signature

Deprecate using `#serialize` with boolean argument

2 files changed, 85 insertions, 10 deletions

diff --git a/lib/axlsx/package.rb b/lib/axlsx/package.rb
index 3afb6f8d..d9865a48 100644
--- a/lib/axlsx/package.rb
+++ b/lib/axlsx/package.rb
@@ -74,13 +74,14 @@ module Axlsx
     # Serialize your workbook to disk as an xlsx document.
     #
     # @param [String] output The name of the file you want to serialize your package to
-    # @param [Boolean] confirm_valid Validate the package prior to serialization.
-    # @param [String, nil] zip_command When `nil`, `#serialize` with RubyZip to
+    # @param [Hash] options
+    # @option options [Boolean] :confirm_valid Validate the package prior to serialization.
+    # @option options [String] :zip_command When `nil`, `#serialize` with RubyZip to
     #   zip the XLSX file contents. When a String, the provided zip command (e.g.,
     #   "zip") is used to zip the file contents (may be faster for large files)
     # @return [Boolean] False if confirm_valid and validation errors exist. True if the package was serialized
     # @note A tremendous amount of effort has gone into ensuring that you cannot create invalid xlsx documents.
-    #   confirm_valid should be used in the rare case that you cannot open the serialized file.
+    #   options[:confirm_valid] should be used in the rare case that you cannot open the serialized file.
     # @see Package#validate
     # @example
     #   # This is how easy it is to create a valid xlsx file. Of course you might want to add a sheet or two, and maybe some data, styles and charts.
@@ -92,14 +93,15 @@ module Axlsx
     #   p.serialize("example.xlsx")
     #
     #   # Serialize to a file, using a system zip binary
-    #   p.serialize("example.xlsx", false, zip_command: "zip")
-    #   p.serialize("example.xlsx", false, zip_command: "/path/to/zip")
-    #   p.serialize("example.xlsx", false, zip_command: "zip -1")
+    #   p.serialize("example.xlsx", zip_command: "zip", confirm_valid: false)
+    #   p.serialize("example.xlsx", zip_command: "/path/to/zip")
+    #   p.serialize("example.xlsx", zip_command: "zip -1")
     #
     #   # Serialize to a stream
     #   s = p.to_stream()
     #   File.open('example_streamed.xlsx', 'w') { |f| f.write(s.read) }
-    def serialize(output, confirm_valid=false, zip_command: nil)
+    def serialize(output, options = {}, secondary_options = nil)
+      confirm_valid, zip_command = parse_serialize_options(options, secondary_options)
       return false unless !confirm_valid || self.validate.empty?
       zip_provider = if zip_command
                        ZipCommand.new(zip_command)
@@ -359,5 +361,28 @@ module Axlsx
       rels.lock
       rels
     end
+
+    # Parse the arguments of `#serialize`
+    # @return [Boolean, (String or nil)] Returns an array where the first value is
+    #   `confirm_valid` and the second is the `zip_command`.
+    # @private
+    def parse_serialize_options(options, secondary_options)
+      if secondary_options
+        warn "[DEPRECATION] Axlsx::Package#serialize with 3 arguments is deprecated. " +
+          "Use keyword args instead e.g., package.serialize(output, confirm_valid: false, zip_command: 'zip')"
+      end
+      if options.is_a?(Hash)
+        options.merge!(secondary_options || {})
+        invalid_keys = options.keys - [:confirm_valid, :zip_command]
+        if invalid_keys.any?
+          raise ArgumentError.new("Invalid keyword arguments: #{invalid_keys}")
+        end
+        [options.fetch(:confirm_valid, false), options.fetch(:zip_command, nil)]
+      else
+        warn "[DEPRECATION] Axlsx::Package#serialize with confirm_valid as a boolean is deprecated. " +
+          "Use keyword args instead e.g., package.serialize(output, confirm_valid: false)"
+        parse_serialize_options((secondary_options || {}).merge(confirm_valid: options), nil)
+      end
+    end
   end
 end
diff --git a/test/tc_package.rb b/test/tc_package.rb
index ac79b487..d313c2bd 100644
--- a/test/tc_package.rb
+++ b/test/tc_package.rb
@@ -129,25 +129,28 @@ class TestPackage < Test::Unit::TestCase
   def test_serialization
     @package.serialize(@fname)
     assert_zip_file_matches_package(@fname, @package)
+    assert_created_with_rubyzip(@fname, @package)
     File.delete(@fname)
   end
 
   def test_serialization_with_zip_command
-    @package.serialize(@fname, false, zip_command: "zip")
+    @package.serialize(@fname, zip_command: "zip")
     assert_zip_file_matches_package(@fname, @package)
+    assert_created_with_zip_command(@fname, @package)
     File.delete(@fname)
   end
 
   def test_serialization_with_zip_command_and_absolute_path
     fname = "#{Dir.tmpdir}/#{@fname}"
-    @package.serialize(fname, false, zip_command: "zip")
+    @package.serialize(fname, zip_command: "zip")
     assert_zip_file_matches_package(fname, @package)
+    assert_created_with_zip_command(fname, @package)
     File.delete(fname)
   end
 
   def test_serialization_with_invalid_zip_command
     assert_raises Axlsx::ZipCommand::ZipError do
-      @package.serialize(@fname, false, zip_command: "invalid_zip")
+      @package.serialize(@fname, zip_command: "invalid_zip")
     end
   end
 
@@ -156,6 +159,53 @@ class TestPackage < Test::Unit::TestCase
     package.send(:parts).each{ |part| zf.get_entry(part[:entry]) }
   end
 
+  def assert_created_with_rubyzip(fname, package)
+    assert_equal 2098, get_mtime(fname, package).year, "XLSX files created with RubyZip have 2098 as the file mtime"
+  end
+
+  def assert_created_with_zip_command(fname, package)
+    assert_equal Time.now.utc.year, get_mtime(fname, package).year, "XLSX files created with a zip command have the current year as the file mtime"
+  end
+
+  def get_mtime(fname, package)
+    zf = Zip::File.open(fname)
+    part = package.send(:parts).first
+    entry = zf.get_entry(part[:entry])
+    entry.mtime.utc
+  end
+
+  def test_serialization_with_deprecated_argument
+    warnings = capture_warnings do
+      @package.serialize(@fname, false)
+    end
+    assert_equal 1, warnings.size
+    assert_includes warnings.first, "confirm_valid as a boolean is deprecated"
+    File.delete(@fname)
+  end
+
+  def test_serialization_with_deprecated_three_arguments
+    warnings = capture_warnings do
+      @package.serialize(@fname, true, zip_command: "zip")
+    end
+    assert_zip_file_matches_package(@fname, @package)
+    assert_created_with_zip_command(@fname, @package)
+    assert_equal 2, warnings.size
+    assert_includes warnings.first, "with 3 arguments is deprecated"
+    File.delete(@fname)
+  end
+
+  def capture_warnings(&block)
+    original_warn = Kernel.instance_method(:warn)
+    warnings = []
+    Kernel.send(:define_method, :warn) { |string| warnings << string }
+    block.call
+    original_verbose = $VERBOSE
+    $VERBOSE = nil
+    Kernel.send(:define_method, :warn, original_warn)
+    $VERBOSE = original_verbose
+    warnings
+  end
+
   # See comment for Package#zip_entry_for_part
   def test_serialization_creates_identical_files_at_any_time_if_created_at_is_set
     @package.core.created = Time.now