Fix #231: Minimize empty elements with lengthy attributes #233
Conversation
|
The build failure is unrelated to the changes in this PR. |
| val pp = new xml.PrettyPrinter(4, 2, minimizeEmpty = true) | ||
| val x = <a b="c"/> | ||
| val formatted = pp.format(x) | ||
| assertEquals(x, XML.loadString(formatted)) |
There was a problem hiding this comment.
Shouldn't this test be comparing strings? With XML.loadString, I think you're just verifying the XML structure is preserved from the original.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| val pp = new xml.PrettyPrinter(4, 2, minimizeEmpty = true) | ||
| val x = <abcdefg/> | ||
| val formatted = pp.format(x) | ||
| assertEquals(x, XML.loadString(formatted)) |
There was a problem hiding this comment.
Same here. The test should be of the string output from PrettyPrinter.format.
|
I may try to circle back and refactor this change and even add more tests, but it does fix the defect. Lord knows, this pretty printer code needs some work (it ain't pretty). Thanks for doing this, Hosam! |
No description provided.