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

Update XML with recent changes for JSONObject optLong vs getLong #790

Closed
stleary opened this issue Oct 13, 2023 · 6 comments
Closed

Update XML with recent changes for JSONObject optLong vs getLong #790

stleary opened this issue Oct 13, 2023 · 6 comments

Comments

@stleary
Copy link
Owner

stleary commented Oct 13, 2023

See #783 and apply the same updates to XML.java for consistent behavior.
Post here if you have questions about how to implement the changes.

@rafu01
Copy link

rafu01 commented Oct 13, 2023

I would like to work on this

rudrajyotib added a commit to rudrajyotib/JSON-java that referenced this issue Oct 14, 2023
Moved the code logic to a common utility to de-duplicate.
@rudrajyotib
Copy link
Contributor

@stleary - I have raised a PR for this. The number conversion and potential number check is moved to a separate utility and that has been used in both parsers.
@rafu01 - I already had the code pattern chalked out, and have implemented. Please go through the PR and let me know if you had thought it to be implemented other way.

@stleary
Copy link
Owner Author

stleary commented Oct 14, 2023

Comments have been added to the PR

@rudrajyotib
Copy link
Contributor

@stleary - Adding these changes into XML is going to affect some of the existing unit test cases.

e.g.,

/**
* JSON string with lost leading zero and converted "True" to true. See test
* result in comment below.
*/
@test
public void testToJSONArray_jsonOutput() {
final String originalXml = "011000<item id="01"/><title>True</title>";
final String expectedJsonString = "["root",["id","01"],["id",1],["id","00"],["id",0],["item",{"id":"01"}],["title",true]]";
final JSONArray actualJsonOutput = JSONML.toJSONArray(originalXml, false);
assertEquals(expectedJsonString, actualJsonOutput.toString());
}

In the comment of the test case, it is mentioned that leading zeros being removed is the expectation. But, in the test case expectation string, leading zeros are kept intact. This is caused by the present implementation, which throws NumberFormatException while parsing number with leading zeros and numbers are parsed as strings.

And, when this logic will be replaced by the new implementation, numbers with leading zeros will be parsed as numbers, and will reflect in the resultant JSON object. This probably is the intended behavior.

Question lies, shall we go ahead and break the existing test cases, or maintain status quo?

@stleary
Copy link
Owner Author

stleary commented Oct 18, 2023

@rudrajyotib Thanks for bringing this up.
Not sure about the relevance of the comment, it looks like its just documenting the test behavior.
The general rule is don't break the unit tests. But sometimes it has to be done.
XML is a special case because of the imperfect transformation with JSON, and by extension so is JSONML, so sometimes we can allow for more flexibility in the behavior.
I will take a look and respond later today.

rudrajyotib added a commit to rudrajyotib/JSON-java that referenced this issue Oct 19, 2023
For now the code remains duplicated in JSON and XML parsers.
Unit test cases updated to comply with number expectations.
@stleary
Copy link
Owner Author

stleary commented Oct 21, 2023

Fixed in #794

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

No branches or pull requests

3 participants