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

[jruby] Different behaviour between jruby nokogiri and C nokogiri when formatting xml #771

Closed
tychobrailleur opened this issue Sep 27, 2012 · 4 comments

Comments

@tychobrailleur
Copy link
Contributor

When formatting xml, JRuby Nokogiri has a different behaviour to the C extension:

greystones:/tmp $ cat test.rb
require 'nokogiri'

doc = Nokogiri::XML("<foo>bar</foo>") do |c|
  c.noblanks
end

puts doc.to_xml(indent: 2)

When using the C extension:

greystones:/tmp $ rvm use 1.9.3
Using /home/sebastien/.rvm/gems/ruby-1.9.3-p194
greystones:/tmp $ ruby test.rb 
<?xml version="1.0"?>
<foo>bar</foo>

the text node is compact, i.e. displayed without whitespace around.

When using the Java extension:

greystones:/tmp $ rvm use jruby-head
Using /home/sebastien/.rvm/gems/jruby-head
greystones:/tmp $ ruby test.rb 
<?xml version="1.0"?>
<foo>
  bar
</foo>

the text node is on its own line, indented, with no way of compacting it.

The discrepancy causes apps that produces XML behave differently with MRI and JRuby.

tychobrailleur added a commit to tychobrailleur/nokogiri that referenced this issue Sep 27, 2012
The Java and C extensions are showing discrepancies when formatting xml: when using the `noblank` option, text nodes are indented and put on a newline by the Java extension, whereas the C one favours a compact approach with no whitespace nor indentation around the text node.  This commit forces the Java extension to behave like the C extension by *never* indenting and “newlining” the text nodes, whether `noblank` is used or not — in line with the observed C extension behaviour.
@jvshahid
Copy link
Member

hi @tychobrailleur, thanks for reporting the bug and fixing it. Can you open a pull request so I can merge your test case and the fix ? Thanks.

@jvshahid
Copy link
Member

nm, i found the pull request.

@tychobrailleur
Copy link
Contributor Author

@jvshahid Sorry, I should have given a reference to the PR here. For the record, here is the pull request: #772

yokolet added a commit that referenced this issue Oct 5, 2012
Fix issue #771 I confirmed the change doesn't break other stuffs. Thanks for fixing this!
@yokolet
Copy link
Member

yokolet commented Oct 5, 2012

@tychobrailleur Thanks for the fix.

@yokolet yokolet closed this as completed Oct 5, 2012
flavorjones added a commit that referenced this issue Oct 5, 2012
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

No branches or pull requests

3 participants