-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Pretty print XML #694
Pretty print XML #694
Conversation
@DeaneOC Nice work. Can you please make the following changes?
|
Thanks @stleary In terms of point one and two, I have JavaDoc comments on the three new methods that are public:
You can currently call toString three different ways so I thought three new public methods that can also take an indent factor were appropriate. The two methods below are private as users should not be able to call them directly, so I have not added JavaDocs, if you would like me to do so let me know:
I believe the current restrictions on method access are correct, three public and two private as mentioned above but if you believe otherwise let me know. Point 3: I have altered test three to use the resource, I was aware it was very long so very happy to make that change. Thanks for getting back to me so quickly and let me know what you think about the above comments so I can make and required changes. |
@DeaneOC Thanks for the update. Yes, please add Javadocs to all new methods, not just the public ones. |
…etty-Print-XML-Functionality
Changes look good, but one of the new unit tests is failing when run from a Windows box: |
Hi, it was to do with the System (mac vs windows) line separators being different. It has now been updated to work for both. Thanks @stleary |
What problem does this code solve? Risks Changes to the API? Will this require a new release? Should the documentation be updated? Does it break the unit tests? Was any code refactored in this commit? Review status Starting 3-day comment window |
"}" ; | ||
JSONObject jsonObject = new JSONObject(str); | ||
String xmlString = XML.toString(jsonObject, "Outer", 1); | ||
System.out.println(xmlString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be better to replace the system out with assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@javadev Can you clarify where you saw this in the code? I cannot find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, the code is outdated. The current implementation has assertEquals
.
This feature was requested some time ago on #593, there are now methods that allow users to pass an
indentFactor
to thetoString
methods in the XML class and this will return a Pretty Printer XML String with spaces based onindentFactor
.This is done similarly to the JSONObject model.
Previous XML result with no
indentFactor
specified:<employee><name>sonoo</name><salary>56000</salary><married>true</married></employee>
New result with
indentFactor
set as 2:Contributed on behalf of the @opencastsoftware open source team. 👋