Skip to content

optLong vs getLong inconsistencies#783

Merged
stleary merged 4 commits into
stleary:masterfrom
rudrajyotib:master
Oct 13, 2023
Merged

optLong vs getLong inconsistencies#783
stleary merged 4 commits into
stleary:masterfrom
rudrajyotib:master

Conversation

@rudrajyotib

Copy link
Copy Markdown
Contributor

For exponential decimal conversion, number is not touched. Leading zeros removed from numeric number strings before converting to number.

For exponential decimal conversion, number is not touched.
Leading zeros removed from numeric number strings before converting to number.
@stleary stleary changed the title #653 - optLong vs getLong inconsistencies optLong vs getLong inconsistencies Oct 7, 2023
@stleary

stleary commented Oct 7, 2023

Copy link
Copy Markdown
Owner

What problem does this code solve?
optLong() and getLong() may produce different results depending on whether leading zeros are present in the value being retrieved.

Risks
Low

Changes to the API?
Yes. stringToNumber() behavior was modified. This may change how the calling methods behave when the value contains leading zeros. It is not believed that this will be a common occurrence.

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
No. New unit tests were added.

Was any code refactored in this commit?
Minimal refactoring of existing code was done (parameter name change, etc)

Review status
APPROVED

Starting 3-day comment window.

Comment thread src/main/java/org/json/JSONObject.java Outdated
Comment thread src/main/java/org/json/JSONObject.java Outdated
@stleary

stleary commented Oct 9, 2023

Copy link
Copy Markdown
Owner

Merge pending resolution of comments.

Comment thread src/main/java/org/json/JSONObject.java Outdated
Comment thread src/test/java/org/json/junit/JSONObjectNumberTest.java
Comment thread src/main/java/org/json/JSONObject.java Outdated
Comment thread src/main/java/org/json/JSONObject.java
Comment thread src/main/java/org/json/JSONObject.java Outdated
@rudrajyotib

Copy link
Copy Markdown
Contributor Author

@stleary @johnjaylward - can you review the latest commits please.

@johnjaylward johnjaylward left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These changes look good now I think. @stleary if we want to maintain similar processing for XML, these changes will need to be copied over to the XML class

@stleary

stleary commented Oct 13, 2023

Copy link
Copy Markdown
Owner

@rudrajyotib Nice work addressing the comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants