From 759f1261000f125a47e11b58905afc4fea8b72e7 Mon Sep 17 00:00:00 2001 From: Ryan Winograd Date: Thu, 20 Aug 2020 09:16:23 -0500 Subject: Deprecate using `#serialize` with boolean argument Update `Axlsx::Package#serialize` to accept the second argument as a boolean (being deprecated) or an options hash. In order to transition toward using keyword arguments for `Axlsx::Package#serialize`, change the documented method signature to an options hash, while still parsing the second argument as `confirm_valid` if a boolean is provided (in which case we also warn the user that a boolean argument is deprecated). --- lib/axlsx/package.rb | 28 ++++++++++++++++++++++++---- test/tc_package.rb | 26 +++++++++++++++++++++++--- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/lib/axlsx/package.rb b/lib/axlsx/package.rb index 3afb6f8d..d9636536 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. @@ -99,7 +100,8 @@ module Axlsx # # 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 = {}) + confirm_valid, zip_command = parse_serialize_options(options) return false unless !confirm_valid || self.validate.empty? zip_provider = if zip_command ZipCommand.new(zip_command) @@ -359,5 +361,23 @@ 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) + if options.is_a?(Hash) + 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)" + [options, nil] + end + end end end diff --git a/test/tc_package.rb b/test/tc_package.rb index 18c08f5c..a6e31f88 100644 --- a/test/tc_package.rb +++ b/test/tc_package.rb @@ -133,21 +133,21 @@ class TestPackage < Test::Unit::TestCase 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) 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) 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 +156,26 @@ class TestPackage < Test::Unit::TestCase package.send(:parts).each{ |part| zf.get_entry(part[:entry]) } 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" + end + + def capture_warnings(&block) + original_warn = Kernel.method(:warn) + warnings = [] + Kernel.define_method(:warn){ |string| warnings << string } + block.call + original_verbose = $VERBOSE + $VERBOSE = nil + Kernel.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 -- cgit v1.2.3 From dcce5db8a653a099d3c9a18cb84baa7b989199fd Mon Sep 17 00:00:00 2001 From: Ryan Winograd Date: Thu, 20 Aug 2020 19:14:02 -0500 Subject: Fix for older rubies? --- test/tc_package.rb | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/test/tc_package.rb b/test/tc_package.rb index a6e31f88..1574c8b8 100644 --- a/test/tc_package.rb +++ b/test/tc_package.rb @@ -165,15 +165,21 @@ class TestPackage < Test::Unit::TestCase end def capture_warnings(&block) - original_warn = Kernel.method(:warn) - warnings = [] - Kernel.define_method(:warn){ |string| warnings << string } - block.call - original_verbose = $VERBOSE - $VERBOSE = nil - Kernel.define_method(:warn, &original_warn) - $VERBOSE = original_verbose - warnings + # Only capture warnings on versions of ruby that expose `:define_method` as + # a public method + if Kernel.respond_to?(:define_method) + original_warn = Kernel.method(:warn) + warnings = [] + Kernel.define_method(:warn){ |string| warnings << string } + block.call + original_verbose = $VERBOSE + $VERBOSE = nil + Kernel.define_method(:warn, &original_warn) + $VERBOSE = original_verbose + warnings + else + &block.call + end end # See comment for Package#zip_entry_for_part -- cgit v1.2.3 From 2f2264dd98595cc71b8fd2df15490de8de57265a Mon Sep 17 00:00:00 2001 From: Ryan Winograd Date: Thu, 20 Aug 2020 21:55:26 -0500 Subject: Actual fix for older rubies --- test/tc_package.rb | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/test/tc_package.rb b/test/tc_package.rb index 1574c8b8..628d20ae 100644 --- a/test/tc_package.rb +++ b/test/tc_package.rb @@ -165,21 +165,15 @@ class TestPackage < Test::Unit::TestCase end def capture_warnings(&block) - # Only capture warnings on versions of ruby that expose `:define_method` as - # a public method - if Kernel.respond_to?(:define_method) - original_warn = Kernel.method(:warn) - warnings = [] - Kernel.define_method(:warn){ |string| warnings << string } - block.call - original_verbose = $VERBOSE - $VERBOSE = nil - Kernel.define_method(:warn, &original_warn) - $VERBOSE = original_verbose - warnings - else - &block.call - end + original_warn = Kernel.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 -- cgit v1.2.3 From 358de36ce6a85cfd85c4b7223e71375dcd074a1a Mon Sep 17 00:00:00 2001 From: Ryan Winograd Date: Fri, 28 Aug 2020 18:20:47 -0500 Subject: Update doc to reflect preferred new public interface --- lib/axlsx/package.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/axlsx/package.rb b/lib/axlsx/package.rb index d9636536..6b55091b 100644 --- a/lib/axlsx/package.rb +++ b/lib/axlsx/package.rb @@ -93,9 +93,9 @@ 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() -- cgit v1.2.3 From c2ac23536dd73ae631f65a45145be170a1186e70 Mon Sep 17 00:00:00 2001 From: Ryan Winograd Date: Fri, 28 Aug 2020 21:14:44 -0500 Subject: Assert how contents are zipped Previously we tested that either rubyzip or shelling out to zip produced the expected xlsx file, but we never explicitly checked whether rubyzip or shell zip was used. I noticed that rubyzip always sets a far future date, whereas `zip` uses today's date. I'm using this as a heuristic to determine which zip method was used. --- test/tc_package.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/tc_package.rb b/test/tc_package.rb index 628d20ae..fc48a9eb 100644 --- a/test/tc_package.rb +++ b/test/tc_package.rb @@ -129,12 +129,14 @@ 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, zip_command: "zip") assert_zip_file_matches_package(@fname, @package) + assert_created_with_zip_command(@fname, @package) File.delete(@fname) end @@ -142,6 +144,7 @@ class TestPackage < Test::Unit::TestCase fname = "#{Dir.tmpdir}/#{@fname}" @package.serialize(fname, zip_command: "zip") assert_zip_file_matches_package(fname, @package) + assert_created_with_zip_command(fname, @package) File.delete(fname) end @@ -156,6 +159,21 @@ 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) -- cgit v1.2.3 From 216996bd076f4f3549155774dbccab4b3c634cd0 Mon Sep 17 00:00:00 2001 From: Ryan Winograd Date: Sat, 29 Aug 2020 09:42:47 -0500 Subject: Update #serialize to accept 3 arguments --- lib/axlsx/package.rb | 13 +++++++++---- test/tc_package.rb | 12 ++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/lib/axlsx/package.rb b/lib/axlsx/package.rb index 6b55091b..d9865a48 100644 --- a/lib/axlsx/package.rb +++ b/lib/axlsx/package.rb @@ -100,8 +100,8 @@ module Axlsx # # Serialize to a stream # s = p.to_stream() # File.open('example_streamed.xlsx', 'w') { |f| f.write(s.read) } - def serialize(output, options = {}) - confirm_valid, zip_command = parse_serialize_options(options) + 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) @@ -366,8 +366,13 @@ module Axlsx # @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) + 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}") @@ -376,7 +381,7 @@ module Axlsx 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)" - [options, nil] + parse_serialize_options((secondary_options || {}).merge(confirm_valid: options), nil) end end end diff --git a/test/tc_package.rb b/test/tc_package.rb index fc48a9eb..b151d06f 100644 --- a/test/tc_package.rb +++ b/test/tc_package.rb @@ -180,6 +180,18 @@ class TestPackage < Test::Unit::TestCase 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) -- cgit v1.2.3 From 9968a88579525231d6f3147ee792076ef037463a Mon Sep 17 00:00:00 2001 From: Ryan Winograd Date: Sat, 29 Aug 2020 09:43:02 -0500 Subject: Avoid warning when using `capture_warnings` more than once --- test/tc_package.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/tc_package.rb b/test/tc_package.rb index b151d06f..3e049f5e 100644 --- a/test/tc_package.rb +++ b/test/tc_package.rb @@ -195,13 +195,13 @@ class TestPackage < Test::Unit::TestCase end def capture_warnings(&block) - original_warn = Kernel.method(:warn) + 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) + Kernel.send(:define_method, :warn, original_warn) $VERBOSE = original_verbose warnings end -- cgit v1.2.3