Merge pull request #122 from westonganger/improve_range_error

Improve error for invalid ranges

5 files changed, 73 insertions, 5 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9983d90d..6e5604fa 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@ CHANGELOG
 ---------
 
 - **Unreleased**
+  - [PR #122](https://github.com/caxlsx/caxlsx/pull/122) - Improve error messages when incorrect ranges are provided to `Worksheet#[]`
   - [PR #123](https://github.com/caxlsx/caxlsx/pull/123) - Fix invalid xml when pivot table created with more than one column in data field. Solves [Issue #110](https://github.com/caxlsx/caxlsx/issues/110)
 
 - **September.22.21**: 3.1.1
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/util/constants.rb b/lib/axlsx/util/constants.rb
index 25781983..3d67088c 100644
--- a/lib/axlsx/util/constants.rb
+++ b/lib/axlsx/util/constants.rb
@@ -307,6 +307,12 @@ module Axlsx
   # error message for non 'integerish' value
   ERR_INTEGERISH = "You value must be, or be castable via to_i, an Integer. You provided %s".freeze
 
+  # error message for invalid cell reference
+  ERR_CELL_REFERENCE_INVALID = "Invalid cell definition `%s`".freeze
+
+  # error message for cell reference with last cell missing
+  ERR_CELL_REFERENCE_MISSING_CELL = "Missing cell `%s` for the specified range `%s`".freeze
+
   # Regex to match forbidden control characters
   # The following will be automatically stripped from worksheets.
   #
diff --git a/lib/axlsx/workbook/worksheet/worksheet.rb b/lib/axlsx/workbook/worksheet/worksheet.rb
index 06a5e7f1..cc0e56ae 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, (ERR_CELL_REFERENCE_INVALID % cell_def)
+        elsif parts.first.nil?
+          raise ArgumentError, (ERR_CELL_REFERENCE_MISSING_CELL % [cell_def.split(":").first, cell_def])
+        elsif parts.last.nil?
+          raise ArgumentError, (ERR_CELL_REFERENCE_MISSING_CELL % [cell_def.split(":").last, 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]