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

Initial implementation of XMLParserConfig object for flexible XML Parsing #412

Merged
merged 1 commit into from
Dec 11, 2018

Conversation

johnjaylward
Copy link
Contributor

@johnjaylward johnjaylward commented Mar 22, 2018

What problem does this code solve?

Implements improvement requested in #394 (see also #108, #286, #344). Adds a new configuration class that can be used to change how the XML Parser works.

Risks
Low. Current functionality works as expected.

Changes to the API?
3 new public APIs are available that take advantage of possible

  • public static JSONObject toJSONObject(Reader reader, XMLParserConfiguration config) throws JSONException
  • public static JSONObject toJSONObject(String string, XMLParserConfiguration config) throws JSONException
  • public static String toString(final Object object, final String tagName, final XMLParserConfiguration config)

A private method signature was also changed, but should not affect the API.

Will this require a new release?
Yes

Should the documentation be updated?
Yes

Does it break the unit tests?
No. No new unit tests were created yet, but we probably should add some.

Was any code refactored in this commit?
Not beyond extending the public APIs to use the new configuration object.

Review status
APPROVED

@johnjaylward johnjaylward force-pushed the XmlConfig branch 2 times, most recently from 5fe48ef to 2abcd30 Compare March 22, 2018 02:09
@stleary
Copy link
Owner

stleary commented Mar 22, 2018

@johnjaylward Looks great! Please add a complete set of unit tests to exercise the new functionality.

@johnjaylward
Copy link
Contributor Author

I'm not sure how soon I can get tests together. If someone else knows they have time, then please comment here and generate the pull request. Otherwise it may be some time before I get to them.

@stleary
Copy link
Owner

stleary commented Nov 4, 2018

If more XML config options are added in the future (e.g. #429), the XMLParserConfig constructor param list won't scale, and the current implementation using public final params can't be modified after creating the object. Recommend using fields with getters/setters for the parser config class. I think it is acceptable in this case to have a single empty constructor that creates an object no params set, and require users to set the config params individually before use.

@stleary
Copy link
Owner

stleary commented Dec 8, 2018

Approved, starting 3 day comment window.
Note: The XMLParserConfiguration API is subject to change if additional config params are proposed.

@stleary stleary merged commit 09dddb8 into stleary:master Dec 11, 2018
@srinivasprv
Copy link

Please update the maven jar. This change is not present in latest maven jar.

@stleary
Copy link
Owner

stleary commented Jul 17, 2019

@srinivasprv Thanks for the reminder, will get this started in the next few days.

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