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

Updates javadoc to match actual exceptions thrown. #405

Merged
merged 1 commit into from Mar 11, 2018

Conversation

johnjaylward
Copy link
Contributor

@johnjaylward johnjaylward commented Mar 7, 2018

What problem does this code solve?

this.put(value);
return this;

changes to:

return this.put(value);

Risks
Low. Changes are either documentation only, or semantic to reduce stack by a single dword by reusing the return values. Changes to API methods are not expected to affect existing applications.

Changes to the API?

  • Adds new API methods:
    JSONArray put(float) throws JSONException
    JSONArray put(int, float) throws JSONException
  • Modifies behavior of existing API methods
    JSONArray put(double) : Removes validity check (because put(Object) does)
    JSONArray put(Object): Tests validity before assignment
    JSONArray put(int, Object): tests validity if in-list, add instead of put NULL after end of current list.
    JSONObject(Map): Throws NPE if key is null

Will this require a new release?
No

Should the documentation be updated?
This PR updates the documentation. If some is missed, please note it in the comments or Code Review sections.

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

Was any code refactored in this commit?
Yes. As mentioned above some boxing calls were updated to the newer java5+ <Number>.valueOf instead of using the constructors as recommended by java code practices. Also, some returns were optimized which should not affect execution beyond decreasing stack size by a single dword.

Review status
ACCEPTED

@johnjaylward johnjaylward force-pushed the FixNPE branch 3 times, most recently from 942fad5 to 09e48b9 Compare March 7, 2018 17:33
Also optimizes some boxing statements and returns.
@stleary
Copy link
Owner

stleary commented Mar 8, 2018

Looks good, starting 3 day comment window.

@data-enabler
Copy link

data-enabler commented Apr 29, 2022

JSONObject(Map): Throws NPE if key is null

A bit late here, but I would consider throwing an exception for an input that used to be accepted a breaking API change. In my company's codebase we had some tests that verified this behavior that were broken as a result, and a currently unknown amount of code that could actually be depending on this behavior.

Obviously this ship has already sailed (and rejecting null keys makes much more sense), but can we at least point out the breaking change in the release history documentation? Would be very useful as a heads-up for anyone trying to upgrade from an old version.

@stleary
Copy link
Owner

stleary commented Apr 30, 2022

Moving this comment to new issues

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.

None yet

3 participants