Skip to content
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

Merged
merged 2 commits into from
Apr 28, 2014

Conversation

rui-castro
Copy link
Contributor

Added test for nil in Roo::Excelx#read_hyperlinks to avoid error "NoMethodError (undefined method `text' for nil:NilClass)"

…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|
Copy link
Contributor

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]"?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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)"
@rui-castro
Copy link
Contributor Author

The xpath solution worked fine. I already update the repository.

@yashke
Copy link

yashke commented Apr 28, 2014

Is there anything blocking for this to be merged in?

Empact added a commit that referenced this pull request Apr 28, 2014
Fixes "NoMethodError (undefined method `text' for nil:NilClass)" issue when reading xlsx file (#122)
@Empact Empact merged commit a46611c into roo-rb:master Apr 28, 2014
@Empact
Copy link
Contributor

Empact commented Apr 28, 2014

Nope! Sorry I've been plenty busy.

@yashke
Copy link

yashke commented Apr 28, 2014

Wow, that was quick. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants