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 for ODS reading with nokogiri 1.5.6 #18

Merged
merged 2 commits into from
Mar 4, 2013
Merged

Conversation

dc7kr
Copy link

@dc7kr dc7kr commented Mar 1, 2013

[] operator on XML nodes now requires the namespace to be specified
reading of ODS files was broken in roo 1.10.2 + Nokogiri 1.5.6

Signed-off-by: Karsten Richter [email protected]

[] operator on XML nodes now requires the namespace to be specified
reading of ODS files was broken in roo 1.10.2 + Nokogiri 1.5.6

Signed-off-by: Karsten Richter <[email protected]>
@Empact
Copy link
Contributor

Empact commented Mar 1, 2013

Hey @dc7kr, I see that the tests pass with your change + nokogiri 1.5.6, and fail with your change + nokogiri 1.5.5. But I'm not seeing the attribute namespace operator change in the release notes: https://groups.google.com/forum/?fromgroups=#!topic/nokogiri-talk/0V0hTTdSkh0

Could you post more info about the change in Nokogiri?

@dc7kr
Copy link
Author

dc7kr commented Mar 2, 2013

Hi,

well it's indeed not there as I learned the hard way but exists as test cases of roo tell you ;) - there were a lot of complaints on the Nokogiri side as well as a dot release changed the API in such a way.

I'm not a Nokogiri developer but just tried to figure out why my rails app no longer was able to parse the OpenOffice documents so I can only describe it from my perspective and what I found in their discussions:

The short summary of this (rather undocumented) change is, that the Nokogiri team wanted to achieve consistency between JRuby and CRuby implementations of their XML parsing. It seems that the only way to achieve that was to introduce the namespace in the attribute access on the nodes.

The main discussion of this change (and that it is a feature and not a bug from their perspective) can be found on their issue 712 here: sparklemotion/nokogiri#712

Noteworthy is also the pull request sparklemotion/nokogiri#726 which contains the actual change.

If we want to keep Nokogiri 1.5.5 compatibility maybe we could replace the node[prefixedattr] calls with node.attributes[notprefixedattr].value which is hopefully equivalent but a bit lengthy.

BTW: my change doesn't contain the roo.gemspec change to bump the dependency to 1.5.6 but as 1.5.5 is not compatible with the current implementation we'd have to do that.

@Empact
Copy link
Contributor

Empact commented Mar 3, 2013

Thanks for the info - could you give your proposed 1.5.5 fix a try? Would be best to not be tied to a patch release.

new helper method "safe_attr" to safely retrieve attributes[].value
or nil if the attr doesn't exist

Signed-off-by: Karsten Richter <[email protected]>
@dc7kr
Copy link
Author

dc7kr commented Mar 3, 2013

Hi,

I now reworked my change and tried the approach mentioned above - test cases run through for 1.5.5 and 1.5.6 now.
Helper method "safe_attr" was needed to prevent nil tests all over the code as we now retrieve attributes['name'].value

Empact added a commit that referenced this pull request Mar 4, 2013
Fixes for ODS reading with nokogiri 1.5.6
@Empact Empact merged commit 4b8ab79 into roo-rb:master Mar 4, 2013
@Empact
Copy link
Contributor

Empact commented Mar 4, 2013

Released 1.10.3. Thanks for the patch!

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.

2 participants