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

XML toJSONObject(Reader reader) #400

Merged
merged 1 commit into from
Feb 10, 2018
Merged

XML toJSONObject(Reader reader) #400

merged 1 commit into from
Feb 10, 2018

Conversation

foldvari
Copy link
Contributor

@foldvari foldvari commented Feb 4, 2018

What problem does this code solve?

  • Currently converting XML to JSON can be done using toJSONObject(String string) method only. In case of huge XML-s the need to convert them to string is very memory consuming. This limits usability.
    As JSONTokener is working with a Reader anyway, introducing a new method in the XML class which accepts a Reader is a very simple, risk free change.

Risks

  • Low

Changes to the API?

  • 3 additional methods.

Will this require a new release?

  • No

Should the documentation be updated?

  • Not necessary

Does it break the unit tests?

  • No. Existing unit tests validate this change as well.

Was any code refactored in this commit?

  • XML.java toJSONObject(String string, boolean keepStrings) now converts String to Reader

Review status
ACCEPTED
Starting 3 day comment window

@foldvari foldvari changed the title XML toJSONObject(Readre reader) XML toJSONObject(Reader reader) Feb 4, 2018
Copy link
Contributor

@johnjaylward johnjaylward left a comment

Choose a reason for hiding this comment

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

These changes look good to me. Straight forward and relatively simple.

@stleary
Copy link
Owner

stleary commented Feb 6, 2018

Thanks for the pull request. Accepted, starting comment window.

@stleary stleary merged commit d402a99 into stleary:master Feb 10, 2018
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

Successfully merging this pull request may close these issues.

3 participants