XML optLong/getLong equivalent updates for string to number conversion. #794

rudrajyotib · 2023-10-14T04:37:21Z

[stleary update]: See #790
Moved the code logic to a common utility to de-duplicate.

Moved the code logic to a common utility to de-duplicate.

src/test/java/org/json/junit/JSONMLTest.java

stleary · 2023-10-14T13:19:35Z

src/test/java/org/json/junit/XMLConfigurationTest.java

@@ -733,7 +733,7 @@ public void contentOperations() {
    @Test
    public void testToJSONArray_jsonOutput() {
        final String originalXml = "<root><id>01</id><id>1</id><id>00</id><id>0</id><item id=\"01\"/><title>True</title></root>";
-        final JSONObject expected = new JSONObject("{\"root\":{\"item\":{\"id\":\"01\"},\"id\":[\"01\",1,\"00\",0],\"title\":true}}");
+        final JSONObject expected = new JSONObject("{\"root\":{\"item\":{\"id\":1},\"id\":[1,1,0,0],\"title\":true}}");


Please do not remove existing test cases.

Hello @stleary - hese test cases were misleading. In the expected JSON string, the numbers still had preceding zeros. If we want the values to be treated as numbers, the leading zeros should go. In fact, the test comment also confirms that.

stleary · 2023-10-14T13:19:52Z

src/test/java/org/json/junit/XMLTest.java

@@ -791,7 +791,7 @@ private void compareFileToJSONObject(String xmlStr, String expectedStr) {
    @Test
    public void testToJSONArray_jsonOutput() {
        final String originalXml = "<root><id>01</id><id>1</id><id>00</id><id>0</id><item id=\"01\"/><title>True</title></root>";
-        final JSONObject expectedJson = new JSONObject("{\"root\":{\"item\":{\"id\":\"01\"},\"id\":[\"01\",1,\"00\",0],\"title\":true}}");
+        final JSONObject expectedJson = new JSONObject("{\"root\":{\"item\":{\"id\":1},\"id\":[1,1,0,0],\"title\":true}}");


Please do not remove existing test cases.

Hello @stleary - hese test cases were misleading. In the expected JSON string, the numbers still had preceding zeros. If we want the values to be treated as numbers, the leading zeros should go. In fact, the test comment also confirms that.

stleary · 2023-10-14T13:20:21Z

~~It would be better to open an issue first, which would provide a forum to discuss the changes before submitting a PR.~~
It's not a bad idea, but you will need to provide evidence that no existing functionality has changed in JSONArray, JSONObject, or XML. Just passing the unit tests is not sufficient.

stleary · 2023-10-15T02:13:51Z

Closed due to failing existing unit tests.

stleary · 2023-10-19T02:25:43Z

@rudrajyotib Sorry about that, should not have closed this PR. It still needs work. For example, if a unit test must be broken, call it out and provide a convincing explanation about why it was necessary. Also, I would strongly discourage refactoring large amounts of code. It's better to have duplicate code at first, and then refactor it in a later PR.

rudrajyotib · 2023-10-19T05:02:49Z

@stleary - can you please add the Hacktoberfest tag back. I have reverted all changes that combined feature and refactor. We can now apply the changes one by one.

rudrajyotib · 2023-10-19T08:06:00Z

@stleary - new implementation breaks some existing test cases.
Expected :["root",["id","01"],["id",1],["id","00"],["id",0],["item",{"id":"01"}],["title",true]]
Actual :["root",["id",1],["id",1],["id",0],["id",0],["item",{"id":1}],["title",true]]

This test case being broken is the expected behavior, in my opinion.

I have pushed these changes with duplicate code.

For now the code remains duplicated in JSON and XML parsers. Unit test cases updated to comply with number expectations.

johnjaylward · 2023-10-20T20:31:59Z

@rudrajyotib , There has been a lot of PRs merged to master today. Please do a merge or rebase to get the latest changes.

rudrajyotib · 2023-10-21T12:33:16Z

@stleary - now synced and merged with master.

stleary · 2023-10-21T15:31:50Z

What problem does this code solve?
The changes that were made for optLong() vs getLong() should also be made to XML

Does the code still compile with Java6?
Yes

Risks
Low

Changes to the API?
Affects how XML converts strings to numbers

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
The unit tests were updated for the new behavior

Was any code refactored in this commit?
No

Review status
APPROVED

Starting 3-day comment window

rudrajyotib · 2023-10-25T18:15:25Z

@stleary - can you please keep the hacktoberfest-accepted label.

stleary · 2023-10-25T18:58:14Z

@rudrajyotib I will update your PRs, but I don't think it's necessary since all accepted PRs get an approving review. I only keep the hacktoberfest label for internal tracking. From the website:
"YOUR PR/MRS MUST BE MERGED, HAVE THE “HACKTOBERFEST-ACCEPTED” LABEL, OR HAVE AN OVERALL APPROVING REVIEW."

stleary#790 - Update XML with changes for string to number conversion.

7b2677a

Moved the code logic to a common utility to de-duplicate.

stleary reviewed Oct 14, 2023

View reviewed changes

src/test/java/org/json/junit/JSONMLTest.java Show resolved Hide resolved

stleary reviewed Oct 14, 2023

View reviewed changes

stleary added In review Hacktoberfest labels Oct 14, 2023

stleary changed the title ~~#790 - Update XML with changes for string to number conversion.~~ Update XML with changes for string to number conversion. Oct 14, 2023

stleary closed this Oct 15, 2023

stleary removed In review Hacktoberfest labels Oct 15, 2023

stleary reopened this Oct 19, 2023

Revert changes with feature and refactor together.

1d0775c

stleary#790 - Update XML with changes for string to number conversion.

2374766

For now the code remains duplicated in JSON and XML parsers. Unit test cases updated to comply with number expectations.

stleary added the Hacktoberfest label Oct 19, 2023

Merge branch 'stleary:master' into master

04a4c5a

stleary added Approved - 3-day window hacktoberfest-accepted and removed Hacktoberfest labels Oct 21, 2023

stleary changed the title ~~Update XML with changes for string to number conversion.~~ XML optLong/getLong equivalent updates for string to number conversion. Oct 21, 2023

stleary mentioned this pull request Oct 21, 2023

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

Closed

stleary approved these changes Oct 25, 2023

View reviewed changes

stleary removed the Approved - 3-day window label Oct 25, 2023

stleary added Hacktoberfest and removed hacktoberfest-accepted labels Oct 25, 2023

stleary merged commit caadcba into stleary:master Oct 25, 2023
6 checks passed

stleary added hacktoberfest-accepted and removed Hacktoberfest labels Oct 25, 2023

stleary mentioned this pull request Feb 24, 2024

Problem with optLong, getLong when parsing JSONObject and XML #868

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XML optLong/getLong equivalent updates for string to number conversion. #794

XML optLong/getLong equivalent updates for string to number conversion. #794

rudrajyotib commented Oct 14, 2023 •

edited by stleary

stleary Oct 14, 2023

rudrajyotib Oct 15, 2023

stleary Oct 14, 2023

rudrajyotib Oct 15, 2023

stleary commented Oct 14, 2023 •

edited

stleary commented Oct 15, 2023

stleary commented Oct 19, 2023

rudrajyotib commented Oct 19, 2023

rudrajyotib commented Oct 19, 2023 •

edited

johnjaylward commented Oct 20, 2023

rudrajyotib commented Oct 21, 2023

stleary commented Oct 21, 2023 •

edited

rudrajyotib commented Oct 25, 2023

stleary commented Oct 25, 2023

XML optLong/getLong equivalent updates for string to number conversion. #794

XML optLong/getLong equivalent updates for string to number conversion. #794

Conversation

rudrajyotib commented Oct 14, 2023 • edited by stleary

stleary Oct 14, 2023

Choose a reason for hiding this comment

rudrajyotib Oct 15, 2023

Choose a reason for hiding this comment

stleary Oct 14, 2023

Choose a reason for hiding this comment

rudrajyotib Oct 15, 2023

Choose a reason for hiding this comment

stleary commented Oct 14, 2023 • edited

stleary commented Oct 15, 2023

stleary commented Oct 19, 2023

rudrajyotib commented Oct 19, 2023

rudrajyotib commented Oct 19, 2023 • edited

johnjaylward commented Oct 20, 2023

rudrajyotib commented Oct 21, 2023

stleary commented Oct 21, 2023 • edited

rudrajyotib commented Oct 25, 2023

stleary commented Oct 25, 2023

rudrajyotib commented Oct 14, 2023 •

edited by stleary

stleary commented Oct 14, 2023 •

edited

rudrajyotib commented Oct 19, 2023 •

edited

stleary commented Oct 21, 2023 •

edited