Fix Worksheet#name_to_cell bug which returned reversed row/col indexes

4 files changed, 69 insertions, 5 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b9a0d429..700a6b31 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,6 +1,9 @@
 CHANGELOG
 ---------
 
+- **Unreleased**
+  - Improve error messages when incorrect ranges are provided to `Worksheet#[]`
+
 - **September.22.21**: 3.1.1
   - [PR #107](https://github.com/caxlsx/caxlsx/pull/107) - Add overlap to bar charts
   - [PR #108](https://github.com/caxlsx/caxlsx/pull/108) - Fix gap depth and gap depth validators for bar charts and 3D bar charts
diff --git a/lib/axlsx.rb b/lib/axlsx.rb
index 0c8c3850..4e804513 100644
--- a/lib/axlsx.rb
+++ b/lib/axlsx.rb
@@ -79,11 +79,25 @@ module Axlsx
   # returns the x, y position of a cell
   def self.name_to_indices(name)
     raise ArgumentError, 'invalid cell name' unless name.size > 1
+
+    letters_str = name[/[A-Z]+/]
+
     # capitalization?!?
-    v = name[/[A-Z]+/].reverse.chars.reduce({:base=>1, :i=>0}) do  |val, c|
-      val[:i] += ((c.bytes.first - 64) * val[:base]); val[:base] *= 26; val
+    v = letters_str.reverse.chars.reduce({:base=>1, :i=>0}) do  |val, c|
+      val[:i] += ((c.bytes.first - 64) * val[:base])
+
+      val[:base] *= 26
+
+      next val
     end
-    [v[:i]-1, ((name[/[1-9][0-9]*/]).to_i)-1]
+
+    col_index = (v[:i] - 1)
+
+    numbers_str = name[/[1-9][0-9]*/]
+
+    row_index = (numbers_str.to_i - 1)
+
+    return [col_index, row_index]
   end
 
   # converts the column index into alphabetical values.
diff --git a/lib/axlsx/workbook/worksheet/worksheet.rb b/lib/axlsx/workbook/worksheet/worksheet.rb
index 06a5e7f1..f1ec815c 100644
--- a/lib/axlsx/workbook/worksheet/worksheet.rb
+++ b/lib/axlsx/workbook/worksheet/worksheet.rb
@@ -599,10 +599,20 @@ module Axlsx
     # @return [Cell, Array]
     def [](cell_def)
       return rows[cell_def] if cell_def.is_a?(Integer)
+
       parts = cell_def.split(':').map{ |part| name_to_cell part }
+
       if parts.size == 1
         parts.first
       else
+        if parts.size > 2
+          raise ArgumentError.new("Invalid cell definition `#{cell_def}`")
+        elsif parts.first.nil?
+          raise ArgumentError.new("Missing cell `#{cell_def.split(":").first}` for the specified range `#{cell_def}`")
+        elsif parts.last.nil?
+          raise ArgumentError.new("Missing cell `#{cell_def.split(":").last}` for the specified range `#{cell_def}`")
+        end
+
         range(*parts)
       end
     end
@@ -612,8 +622,12 @@ module Axlsx
     # @return [Cell]
     def name_to_cell(name)
       col_index, row_index = *Axlsx::name_to_indices(name)
+
       r = rows[row_index]
-      r[col_index] if r
+
+      if r
+        return r[col_index] 
+      end
     end
 
     # shortcut method to access styles direclty from the worksheet
@@ -688,11 +702,13 @@ module Axlsx
     def range(*cell_def)
       first, last = cell_def
       cells = []
+
       rows[(first.row.row_index..last.row.row_index)].each do |r|
         r[(first.index..last.index)].each do |c|
           cells << c
         end
       end
+
       cells
     end
 
diff --git a/test/workbook/worksheet/tc_worksheet.rb b/test/workbook/worksheet/tc_worksheet.rb
index 734096b5..c679e18c 100644
--- a/test/workbook/worksheet/tc_worksheet.rb
+++ b/test/workbook/worksheet/tc_worksheet.rb
@@ -493,6 +493,37 @@ class TestWorksheet < Test::Unit::TestCase
     assert_raise(ArgumentError, 'only accept Integer or Float') { @ws.column_widths 2, 7, "-1" }
   end
 
+  def test_ranges
+    @ws.add_row(["foo", "bar"])
+    @ws.add_row(["a", "b", "c", "d", "e"])
+
+    valid_range = "A1:B2"
+    @ws[valid_range]
+
+    valid_range = "A1:E2" ### Missing middle cells (C1 - E1), still allowed
+    @ws[valid_range]
+
+    assert_raise ArgumentError, "Invalid cell definition" do
+      invalid_range_format = "A1:B2:C3"
+      @ws[invalid_range_format]
+    end
+
+    assert_raise ArgumentError, "Missing cell `Z1` for the specified range." do
+      invalid_row_range = "A1:Z1"
+      @ws[invalid_row_range]
+    end
+
+    assert_raise ArgumentError, "Missing cell `D1` for the specified range." do
+      invalid_cell_range = "D1:E2" ### Missing start cell, not allowed
+      @ws[invalid_cell_range]
+    end
+
+    assert_raise ArgumentError, "Missing cell `A99` for the specified range." do
+      invalid_cell_range = "A1:A99" ### Missing end cell, not allowed
+      @ws[invalid_cell_range]
+    end
+  end
+
   def test_protect_range
     assert(@ws.send(:protected_ranges).is_a?(Axlsx::SimpleTypedList))
     assert_equal(0, @ws.send(:protected_ranges).size)
@@ -504,8 +535,8 @@ class TestWorksheet < Test::Unit::TestCase
     @ws.add_row [1, 2, 3]
     assert_nothing_raised {@ws.protect_range(@ws.rows.first.cells) }
     assert_equal('A1:C1', @ws.send(:protected_ranges).last.sqref)
-
   end
+
   def test_merge_cells
     @ws.add_row [1,2,3]
     @ws.add_row [4,5,6]