Initial implementation of XMLParserConfig object for flexible XML Parsing#412
Initial implementation of XMLParserConfig object for flexible XML Parsing#412stleary merged 1 commit intostleary:masterfrom
Conversation
johnjaylward
force-pushed
the
XmlConfig
branch
2 times, most recently
from
5fe48ef to
2abcd30
Compare
|
@johnjaylward Looks great! Please add a complete set of unit tests to exercise the new functionality. |
|
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. |
|
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. |
|
Approved, starting 3 day comment window. |
|
Please update the maven jar. This change is not present in latest maven jar. |
|
@srinivasprv Thanks for the reminder, will get this started in the next few days. |
3 participants
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 JSONExceptionpublic static JSONObject toJSONObject(String string, XMLParserConfiguration config) throws JSONExceptionpublic 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