Skip to content

Conversation

@stleary
Copy link
Owner

@stleary stleary commented Aug 9, 2016

Reverts #249

@johnjaylward
Copy link
Contributor

johnjaylward commented Aug 9, 2016

@stleary I don't think this is necessary. See my comment on #260

@stleary
Copy link
Owner Author

stleary commented Aug 9, 2016

What problem does this code solve?
#249 was supposed to be a minor refactoring, but it contains an uncaught bug that breaks the opt*() methods. It needs to be reverted,

Risk
This is considered a low risk change since it is a straight revert to a working stable build. The only code change between the introduction of the refactoring and the revert is #253, which modified XML, JSONML, and JSONTokener, and which changes are believed to be orthogonal to this commit. The unit tests have been improved to catch the regression introduced in the refactor.

Changes to the API?
No change to the API

Will this require a new release?
Yes, the 20160807 Maven release contains this bug.

Should the documentation be updated?
Not required

Change to unit tests?
Yes, stleary/JSON-Java-unit-test#55, after this code is committed.

Note
This pull request is being fast-tracked, so if you have comments, get them in quickly. Will leave the request open for 1 day.

This was referenced Aug 9, 2016
@johnjaylward
Copy link
Contributor

@stleary looking at the reversal, this does not solve the issue.

@stleary
Copy link
Owner Author

stleary commented Aug 9, 2016

@johnjaylward please elaborate.

@johnjaylward
Copy link
Contributor

the functions optBoolean, optLong, optInt, etc were calling the get method internally before the original changes were made that this is reverting.

@johnjaylward
Copy link
Contributor

https://github.com/stleary/JSON-java/blob/master/JSONObject.java#L513

it is still calling get() without a try-catch block, so the issue in #260 is not solved.

@johnjaylward
Copy link
Contributor

oh nevermind. I thought this was merged already, but I now see it's not. I was mistaken when I saw the closed status above.

@stleary
Copy link
Owner Author

stleary commented Aug 9, 2016

No worries, let me know if you see any other reason not to revert, am otherwise strongly inclined to proceed.

@johnjaylward
Copy link
Contributor

I ran the tests against the reverted branch just now, and they all seem to pass, while there are failures on master. At this time I see no reason to hold the revert.

@johnjaylward
Copy link
Contributor

You may want to just add a new commit after the revert to correct the comment fixes that @Simulant87 made in #249. Those are valid fixes.

@Simulant87
Copy link
Contributor

@johnjaylward is right. changing

this.get(key);

to

this.opt(key);

in all the optXXX methods fixes the bug. I think I can submit a new merge-request to fix this bug in 12 hours, if you can wait for this. Otherwise someone else can do the fix.

@stleary stleary merged commit 154cfda into master Aug 10, 2016
@stleary stleary deleted the revert-249-master branch July 1, 2025 13:22
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.

4 participants