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

Pretty print XML #694

Merged
merged 10 commits into from Oct 14, 2022
Merged

Conversation

DeaneOC
Copy link
Contributor

@DeaneOC DeaneOC commented Oct 6, 2022

This feature was requested some time ago on #593, there are now methods that allow users to pass an indentFactorto the toString methods in the XML class and this will return a Pretty Printer XML String with spaces based on indentFactor.

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:

<employee>
  <name>sonoo</name>
  <salary>56000</salary>
  <married>true</married>
</employee>

Contributed on behalf of the @opencastsoftware open source team. 👋

@stleary stleary changed the title Fix #593 Pretty print functionality now available with XML Pretty print XML Oct 6, 2022
@stleary
Copy link
Owner

stleary commented Oct 6, 2022

@DeaneOC Nice work. Can you please make the following changes?

  • New methods should have JavaDocs, especially for new parameters.
  • Make sure new methods actually need to be public, or else use more restrictive access.
  • Unit test very large strings should be stored in src/test/resources. You can search for 'issue537' for an example of how to do this.

@DeaneOC
Copy link
Contributor Author

DeaneOC commented Oct 7, 2022

@DeaneOC Nice work. Can you please make the following changes?

  • New methods should have JavaDocs, especially for new parameters.
  • Make sure new methods actually need to be public, or else use more restrictive access.
  • Unit test very large strings should be stored in src/test/resources. You can search for 'issue537' for an example of how to do this.

Thanks @stleary

In terms of point one and two, I have JavaDoc comments on the three new methods that are public:

toString(object, indentFactor)
toString(object, tagLine, indentFactor)
toString(object, tagline, config, indentFactor)

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:

toString(object, tagline, config, indentFactor, indent)
indent(indent)

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.

@stleary
Copy link
Owner

stleary commented Oct 7, 2022

@DeaneOC Thanks for the update. Yes, please add Javadocs to all new methods, not just the public ones.

@DeaneOC
Copy link
Contributor Author

DeaneOC commented Oct 7, 2022

@DeaneOC Thanks for the update. Yes, please add Javadocs to all new methods, not just the public ones.

Javadocs added. Thanks @stleary

@stleary
Copy link
Owner

stleary commented Oct 7, 2022

Changes look good, but one of the new unit tests is failing when run from a Windows box:
org.json.junit.XMLTest > testIndentComplicatedJsonObjectWithArrayAndWithConfig FAILED org.junit.ComparisonFailure at XMLTest.java:1235

@DeaneOC
Copy link
Contributor Author

DeaneOC commented Oct 10, 2022

Changes look good, but one of the new unit tests is failing when run from a Windows box: org.json.junit.XMLTest > testIndentComplicatedJsonObjectWithArrayAndWithConfig FAILED org.junit.ComparisonFailure at XMLTest.java:1235

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

@stleary
Copy link
Owner

stleary commented Oct 10, 2022

What problem does this code solve?
Add support for XML pretty print

Risks
Low

Changes to the API?
New public API methods

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
No. Unit tests were added.

Was any code refactored in this commit?
No

Review status
APPROVED

Starting 3-day comment window

"}" ;
JSONObject jsonObject = new JSONObject(str);
String xmlString = XML.toString(jsonObject, "Outer", 1);
System.out.println(xmlString);
Copy link
Contributor

@javadev javadev Oct 14, 2022

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.

Copy link
Owner

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.

Copy link
Contributor

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.

@stleary stleary merged commit 1be6ee3 into stleary:master Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants