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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Oct 25, 2023
Merged

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

merged 4 commits into from
Oct 25, 2023

Conversation

rudrajyotib
Copy link
Contributor

@rudrajyotib rudrajyotib commented Oct 14, 2023 •
edited by stleary
Loading

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

@rudrajyotib
stleary#790 - Update XML with changes for string to number conversion.
7b2677a 
Moved the code logic to a common utility to de-duplicate.
stleary
stleary reviewed Oct 14, 2023
View reviewed changes
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}}");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not remove existing test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
stleary reviewed Oct 14, 2023
View reviewed changes
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}}");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not remove existing test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Owner

stleary commented Oct 14, 2023 •
edited
Loading

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 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
Copy link
Owner

stleary commented Oct 15, 2023

Closed due to failing existing unit tests.

@stleary stleary closed this Oct 15, 2023
@stleary
Copy link
Owner

stleary commented Oct 19, 2023

@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.

@stleary stleary reopened this Oct 19, 2023
@rudrajyotib
Copy link
Contributor Author

@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
Copy link
Contributor Author

rudrajyotib commented Oct 19, 2023 •
edited
Loading

@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.

@rudrajyotib
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.
@johnjaylward
Copy link
Contributor

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

@rudrajyotib
Copy link
Contributor Author

@stleary - now synced and merged with master.

@stleary 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
Copy link
Owner

stleary commented Oct 21, 2023 •
edited
Loading

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

@stleary stleary mentioned this pull request Oct 21, 2023
Closed
@stleary stleary merged commit caadcba into stleary:master Oct 25, 2023
6 checks passed
@rudrajyotib
Copy link
Contributor Author

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

@stleary
Copy link
Owner

stleary commented Oct 25, 2023

@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 stleary mentioned this pull request Feb 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stleary stleary stleary approved these changes

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rudrajyotib @stleary @johnjaylward