roo-rb / roo

Roo provides an interface to spreadsheets of several sorts.
MIT License
2.78k stars 503 forks source link

Problem with broken links #533

Closed estani closed 1 year ago

estani commented 4 years ago

If broken links are found (references to non existing sheets, DB, etc) a set of problems arise, e.g.

 > worksheet.sheet(1).cell(1,1)
'assuming this works'
 > worksheet.sheet(1).a1
NoMethodError (undefined method `split' for nil:NilClass)

The main culprit is: File: ../../.rbenv/versions/2.5.7/lib/ruby/gems/2.5.0/gems/roo-2.8.3/lib/roo/excelx/workbook.rb 31: def defined_names 32: doc.xpath('//definedName').each_with_object({}) do |defined_name, hash| 33: # "Sheet1!$C$5" 34: sheet, coordinates = defined_name.text.split('!$', 2) 35: col, row = coordinates.split('$') 36: name = defined_name['name'] 37: hash[name] = Label.new(name, sheet, row, col) 38: end 39: end

defined_name.text turns out to be "Datenblatt!#REF!" which can't be properly split at !$

The main question is why !$ is used instead of simply ! which seems more proper. In any case a more robust solution (which I'm using now) would be:

File: ../../.rbenv/versions/2.5.7/lib/ruby/gems/2.5.0/gems/roo-2.8.3/lib/roo/excelx/workbook.rb 31: def defined_names 32: doc.xpath('//definedName').each_with_object({}) do |defined_name, hash| 33: # "Sheet1!$C$5" 34: sheet, coordinates = defined_name.text.split('!$', 2) 35: next unless coordinates 36: 37: col, row = coordinates.split('$') 38: name = defined_name['name'] 39: hash[name] = Label.new(name, sheet, row, col) 40: end 41: end

(a pull request costs me much more than writing this, and I have no Idea why !$ is important, as I don't know the format)

coorasse commented 3 years ago

Formatted lib/roo/excelx/workbook.rb:

current version

def defined_names
  doc.xpath('//definedName').each_with_object({}) do |defined_name, hash|
    # "Sheet1!$C$5"
    sheet, coordinates = defined_name.text.split('!$', 2)
    col, row = coordinates.split('$')
    name = defined_name['name']
    hash[name] = Label.new(name, sheet, row, col)
  end
end

patched version

def defined_names
  doc.xpath('//definedName').each_with_object({}) do |defined_name, hash|
    # "Sheet1!$C$5"
    sheet, coordinates = defined_name.text.split('!$', 2)
    next unless coordinates
    col, row = coordinates.split('$')
    name = defined_name['name']
    hash[name] = Label.new(name, sheet, row, col)
  end
end