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

Revert "reduces the use of unnecessary exceptions" #261

Merged
merged 1 commit into from
Aug 10, 2016

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.

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.

3 participants