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

Optional type conversion for XML reading #253

Merged
merged 3 commits into from
Aug 1, 2016

Conversation

johnjaylward
Copy link
Contributor

When reading XML and JSONML offer new option to not convert strings. This was mostly from PR #70 and is in reference for many issues brought up about conversions that are unwanted during XML processing, the lastest of which is #243

It expands on pr #70 by bringing the conversion option directly into the XML class and not just in JSONML. It also prefers polymorphism instead of new function names.

Test cases from the original PR will be included in a separate commit in the test project.

@stleary
Copy link
Owner

stleary commented Jul 19, 2016

No problem with the XML changes, but concerned about opening the API to JSONML, JSONObject, and JSONArray to ignore type conversions. Is there any way to do this without changing the public API for those classes?

@johnjaylward
Copy link
Contributor Author

JSONObject and JSONArray should be unaffected. JSONML is another XML representation and should be be consistent with the standard XML class I think. I'm personally unsure why JSONML is not a subclass of the XML class. On the surface, there appears to be a lot of duplicate code between the 2 but I didn't take the time to go through them and reduce duplication.

@stleary
Copy link
Owner

stleary commented Jul 19, 2016

Thanks for clarifying about JSONObject and JSONArray, agreed. I believe we have a certain freedom to work with XML, primarily because it was a candidate for deletion from the library. However, the same may not hold true for JSONML. Don't mind if they diverge a little, but unless there is a compelling argument otherwise, would prefer to avoid changes to the API that are not bug-related.

@johnjaylward
Copy link
Contributor Author

Any existing code should still work as expected with JSONML. The caller would have to specify the new parameter with a true value to get the new parsing. I think the overall benefits of keeping the XML and the JSONML classes with similar APIs and features outweighs not implementing it. At the time of the original PR there also seems to be a decent amount of support for it.

@stleary
Copy link
Owner

stleary commented Jul 19, 2016

Thanks for doing the research, will need a little time to review those PRs.

@stleary
Copy link
Owner

stleary commented Jul 23, 2016

What problem does this code solve?
A number of developers have requested a little more control over XML parsing. This gives the ability to specify parsing of XML values to strings instead of other JSON types.

Changes to the API?
Yes, the XML and JSONML APIs are extended to allow the developer to specify string parsing. The default behavior is still first attempt to parse to JSON types.

Will this require a new release?
This change will be rolled into the next release, which is expected in July or August 2016 timeframe.

Should the documentation be updated?
Yes, since the API is changing, the new behavior should be called out to users. Some additional text in the README would be appropriate.

Change to unit tests?
New unit tests are added, which will only succeed with the code change. See stleary/JSON-Java-unit-test#52

Note
This pull request will be kept open for 7 days, pending possible comment from devs who use XML and JSONML.

@stleary
Copy link
Owner

stleary commented Jul 27, 2016

Adding this to bump the comments. This PR will be kept open for 7 days, in case XML or JSONML users want to comment.

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