Skip to content

Fix #231: Minimize empty elements with lengthy attributes #233

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 25, 2018
Merged

Fix #231: Minimize empty elements with lengthy attributes #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions jvm/src/test/scala/scala/xml/XMLTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,4 +749,20 @@ class XMLTestJVM {
assertEquals("<node>\n <leaf/>\n</node>", pp.format(x))
}

@UnitTest
def issue231: Unit = {
val pp = new xml.PrettyPrinter(4, 2, minimizeEmpty = true)
val x = <a b="c"/>
val formatted = pp.format(x)
assertEquals(x, XML.loadString(formatted))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this test be comparing strings? With XML.loadString, I think you're just verifying the XML structure is preserved from the original.

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'm verifying that there are no extra nodes caused by the formatting, such as the Text node reported in #231.

I didn't want to hard-code a string to avoid having to modify the test if the formatting behaviour changes. For example, if the commented-out todo is fixed, the output may have 3 lines instead of 2.

// current behaviour
pp.format(x) == s"""<a\n  b="c"/>"""
// possible future behaviour
pp.format(x) == s"""<a\n  b="c"\n  />"""

Since the bug is about the XML structure changing (and, in my case, failing schema validation), I think that using XML.loadString is a better option.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok. That's a fair point. The XML structure was getting changed.

There are other tests of PrettyPrinter that compare strings. If history is any guide, I don't think it's a huge risk for the formatting behavior to change often. Again, strings are the output of PrettyPrint.format, so it seems worth checking.

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'm not sure what value a string comparison would add. I think that the functionality of formatting is covered by other tests, e.g. issue90 on line 746. This test also checks that some formatting happened by checking the number of lines in the output.

I'll add the string comparison if you deem it necessary, but I don't think it would add much value.

assertTrue(formatted.trim.lines.length >= 2)
}

@UnitTest
def issue231_withoutAttributes: Unit = {
val pp = new xml.PrettyPrinter(4, 2, minimizeEmpty = true)
val x = <abcdefg/>
val formatted = pp.format(x)
assertEquals(x, XML.loadString(formatted))
Copy link
Member

@ashawley ashawley May 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. The test should be of the string output from PrettyPrinter.format.

}
}
22 changes: 16 additions & 6 deletions shared/src/main/scala/scala/xml/PrettyPrinter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,16 @@ class PrettyPrinter(width: Int, step: Int, minimizeEmpty: Boolean) {
if (childrenAreLeaves(node) && fits(test)) {
makeBox(ind, test)
} else {
val (stg, len2) = startTag(node, pscope)
val etg = endTag(node)
val ((stg, len2), etg) =
if (node.child.isEmpty && minimizeEmpty) {
// force the tag to be self-closing
val firstAttribute = test.indexOf(' ')
val firstBreak = if (firstAttribute != -1) firstAttribute else test.lastIndexOf('/')
((test, firstBreak), "")
} else {
(startTag(node, pscope), endTag(node))
}

if (stg.length < width - cur) { // start tag fits
makeBox(ind, stg)
makeBreak()
Expand All @@ -180,10 +188,12 @@ class PrettyPrinter(width: Int, step: Int, minimizeEmpty: Boolean) {
makeBreak()
}
}*/
makeBox(ind, stg.substring(len2, stg.length))
makeBreak()
traverse(node.child.iterator, node.scope, ind + step)
makeBox(cur, etg)
makeBox(ind, stg.substring(len2, stg.length).trim)
if (etg.nonEmpty) {
makeBreak()
traverse(node.child.iterator, node.scope, ind + step)
makeBox(cur, etg)
}
makeBreak()
} else { // give up
makeBox(ind, test)
Expand Down