-
Notifications
You must be signed in to change notification settings - Fork 502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes "NoMethodError (undefined method `text' for nil:NilClass)" issue when reading xlsx file (https://github.com/Empact/roo/issues/122) #123
Conversation
…ethodError (undefined method `text' for nil:NilClass)"
@@ -539,7 +539,7 @@ def read_hyperlinks(sheet=nil) | |||
[r.attribute('Id').text, r] | |||
end] | |||
@sheet_doc[n].xpath("/xmlns:worksheet/xmlns:hyperlinks/xmlns:hyperlink").each do |h| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about instead enforcing the presence of id in the selector, ala: "/xmlns:worksheet/xmlns:hyperlinks/xmlns:hyperlink[id]"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently this line:
@sheet_doc[n].xpath("/xmlns:worksheet/xmlns:hyperlinks/xmlns:hyperlink").each do |h|
is iterating over each of the elements that match a certain xpath selector, then you're skipping over the ones of those that don't have the id attribute. You can simplify things by only selecting the ones that have id attributes in the first place, with code like this:
@sheet_doc[n].xpath("/xmlns:worksheet/xmlns:hyperlinks/xmlns:hyperlink[id]").each do |h|
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and it seems like a better solution. I'm going to try it and update my fix, if it works.
…yperlink[id]" to avoid the previous error "NoMethodError (undefined method `text' for nil:NilClass)"
The xpath solution worked fine. I already update the repository. |
Is there anything blocking for this to be merged in? |
Fixes "NoMethodError (undefined method `text' for nil:NilClass)" issue when reading xlsx file (#122)
Nope! Sorry I've been plenty busy. |
Wow, that was quick. Thanks! |
Added test for nil in Roo::Excelx#read_hyperlinks to avoid error "NoMethodError (undefined method `text' for nil:NilClass)"