Merge branch 'pr-upstream-599' into matrixify

PR: roo-rb#599

CHANGELOG.md

@@ -1,6 +1,8 @@
 ##  Unreleased
 
 ### Changed/Added
+- Prevent warnings on Ruby 3.1 if finalizer is called twice [586](https://github.com/roo-rb/roo/pull/586)
+- Fix Roo::Base#each_with_pagename degraded at [576](https://github.com/roo-rb/roo/pull/576) [583](https://github.com/roo-rb/roo/pull/583)
 
 ##  [2.10.0] 2023-02-07
 

README.md

@@ -18,7 +18,7 @@ Install as a gem
 Or add it to your Gemfile
 
 ```ruby
-gem "roo", "~> 2.9.0"
+gem "roo", "~> 2.10.0"
 ```
 ## Usage
 

lib/roo/base.rb

@@ -250,10 +250,10 @@ def sheet(index, name = false)
 
   # iterate through all worksheets of a document
   def each_with_pagename
-    Enumerator.new do |yielder|
-      sheets.each do |s|
-        yielder << sheet(s, true)
-      end
+    return to_enum(:each_with_pagename) { sheets.size } unless block_given?
+
+    sheets.each do |s|
+      yield sheet(s, true)
     end
   end
 

lib/roo/excelx/sheet.rb

@@ -42,7 +42,7 @@ def each_row(options = {}, &block)
         @sheet.each_row_streaming do |row|
           break if options[:max_rows] && row_count == options[:max_rows] + options[:offset] + 1
           if block_given? && !(options[:offset] && row_count < options[:offset])
-            block.call(cells_for_row_element(row, options))
+            block.call(cells_for_row_element(row, row_count + 1, options))
           end
           row_count += 1
         end
@@ -101,11 +101,11 @@ def dimensions
       # Take an xml row and return an array of Excelx::Cell objects
       # optionally pad array to header width(assumed 1st row).
       # takes option pad_cells (boolean) defaults false
-      def cells_for_row_element(row_element, options = {})
+      def cells_for_row_element(row_element, row_index, options = {})
         return [] unless row_element
         cell_col = 0
         cells = []
-        @sheet.each_cell(row_element) do |cell|
+        @sheet.each_cell(row_element, row_index) do |cell|
           cells.concat(pad_cells(cell, cell_col)) if options[:pad_cells]
           cells << cell
           cell_col = cell.coordinate.column

lib/roo/excelx/sheet_doc.rb

@@ -43,10 +43,10 @@ def each_row_streaming(&block)
 
       # Yield each cell as Excelx::Cell to caller for given
       # row xml
-      def each_cell(row_xml)
+      def each_cell(row_xml, row_index = nil)
         return [] unless row_xml
-        row_xml.children.each do |cell_element|
-          coordinate = ::Roo::Utils.extract_coordinate(cell_element["r"])
+        row_xml.children.each.with_index(1) do |cell_element, col_index|
+          coordinate = cell_coordinate(cell_element["r"], row_index, col_index)
           hyperlinks = hyperlinks(@relationships)[coordinate]
 
           yield cell_from_xml(cell_element, hyperlinks, coordinate)
@@ -213,13 +213,7 @@ def extract_cells(relationships)
 
         doc.xpath('/worksheet/sheetData/row').each.with_index(1) do |row_xml, ycoord|
           row_xml.xpath('c').each.with_index(1) do |cell_xml, xcoord|
-            r = cell_xml['r']
-            coordinate =
-              if r.nil?
-                ::Roo::Excelx::Coordinate.new(ycoord, xcoord)
-              else
-                ::Roo::Utils.extract_coordinate(r)
-              end
+            coordinate = cell_coordinate(cell_xml['r'], ycoord, xcoord)
 
             cell = cell_from_xml(cell_xml, hyperlinks(relationships)[coordinate], coordinate, empty_cell)
             extracted_cells[coordinate] = cell if cell
@@ -252,6 +246,12 @@ def base_timestamp
       def shared_strings
         @shared.shared_strings
       end
+
+      def cell_coordinate(r, row_index, col_index)
+        return ::Roo::Excelx::Coordinate.new(row_index, col_index) if r.nil?
+
+        ::Roo::Utils.extract_coordinate(r)
+      end
     end
   end
 end

lib/roo/tempdir.rb

@@ -4,7 +4,10 @@ def finalize_tempdirs(object_id)
       if @tempdirs && (dirs_to_remove = @tempdirs[object_id])
         @tempdirs.delete(object_id)
         dirs_to_remove.each do |dir|
-          ::FileUtils.remove_entry(dir)
+          # Pass force=true to avoid an exception (and thus warnings in Ruby 3.1) if dir has
+          # already been removed. This can occur when the finalizer is called both in a forked
+          # child process and in the parent.
+          ::FileUtils.remove_entry(dir, true)
         end
       end
     end

spec/lib/roo/base_spec.rb

@@ -183,10 +183,22 @@ def sheets
   end
 
   describe '#each_with_pagename' do
-    it 'should return an enumerator with all the rows' do
-      each_with_pagename = spreadsheet.each_with_pagename
-      expect(each_with_pagename).to be_a(Enumerator)
-      expect(each_with_pagename.to_a.last).to eq([spreadsheet.default_sheet, spreadsheet])
+    context 'when block given' do
+      it 'iterate with sheet and sheet_name' do
+        sheet_names = []
+        spreadsheet.each_with_pagename do |sheet_name, sheet|
+          sheet_names << sheet_name
+        end
+        expect(sheet_names).to eq ['my_sheet', 'blank sheet']
+      end
+    end
+
+    context 'when called without block' do
+      it 'should return an enumerator with all the rows' do
+        each_with_pagename = spreadsheet.each_with_pagename
+        expect(each_with_pagename).to be_a(Enumerator)
+        expect(each_with_pagename.to_a.last).to eq([spreadsheet.default_sheet, spreadsheet])
+      end
     end
   end
 

test/helpers/test_accessing_files.rb

@@ -36,6 +36,27 @@ def test_finalize
     end
   end
 
+  def test_finalize_twice
+    skip if defined? JRUBY_VERSION
+
+    instance = Class.new { include Roo::Tempdir }.new
+
+    tempdir = instance.make_tempdir(instance, "my_temp_prefix", nil)
+    assert File.exist?(tempdir), "Expected #{tempdir} to initially exist"
+
+    pid = Process.fork do
+      # Inside the forked process finalize does not affect the parent process's state, but does
+      # delete the tempfile on disk
+      instance.finalize_tempdirs(instance.object_id)
+    end
+
+    Process.wait(pid)
+    refute File.exist?(tempdir), "Expected #{tempdir} to have been cleaned up by child process"
+
+    instance.finalize_tempdirs(instance.object_id)
+    refute File.exist?(tempdir), "Expected #{tempdir} to still have been cleaned up"
+  end
+
   def test_cleanup_on_error
     # NOTE: This test was occasionally failing because when it started running
     #       other tests would have already added folders to the temp directory,

test/roo/test_excelx.rb

@@ -350,6 +350,21 @@ def test_implicit_coordinates
     assert_equal 'C2', xlsx.cell('c', 2)
   end
 
+  def test_implicit_coordinates_with_streaming_rows
+    xlsx = roo_class.new(File.join(TESTDIR, 'implicit_coordinates.xlsx'))
+
+    expected_rows = [
+      ['Test'],
+      ['A2', 'B2', 'C2']
+    ]
+
+    index = 0
+    xlsx.each_row_streaming do |row|
+      assert_equal expected_rows[index], row.map(&:value)
+      index += 1
+    end
+  end
+
   def roo_class
     Roo::Excelx
   end