Updates for JSONArray.putAll methods

[johnjaylward]

What problem does this code solve?
The new putAll methods did not implement a way to concatenate JSONArray objects. It was an oversight during the review process, but should probably be allowed. This also adds in a shallow copy constructor for JSONArrays so the solution is a bit more holistic. I also added support for adding Generic Iterable objects which don't have a size function.

• Adds a copy constructor for JSONArray
• Updates the JSONArray.addAll(Object) method to be more lenient
• Adds support for JSONArray.putAll of generic Iterables
• Adds support for JSONArray.putAll of another JSONArray
• Updates some JavaDoc for clarification.
• Updates putAll methods to match other put methods and not wrap the items in the collection/array

Risks
Low. Just closes some gaps that were missed in the initial PR

Changes to the API?
New methods added, but no breaking changes

Will this require a new release?
No

Should the documentation be updated?
Yes. Notes should be added about the new putAll methods and information on expected use cases

Does it break the unit tests?
No. New tests were added to verify existing functionality and show new functions

Was any code refactored in this commit?
Yes. The internal addAll methods created in #529 were modified to optionally wrap the contents of the collection/array instead of always wrapping them.

Review status
APPROVED

[stleary]

No objection to enhancing add_all() and put_all() a bit.
A strong case is needed to justify new constructors.
Clone() looks like it was added by accident, code seems to work fine without it.

[stleary]

Starting 3-day comment window.

[johnjaylward]

do you still want clone removed?

[stleary]

do you still want clone removed?

Yes, it does not seem to be related to the rest of the PR.

[johnjaylward]

@stleary in reference to your note about parity with JSONObject constructors, we currently have this one that does a partial copy: https://github.com/stleary/JSON-java/blob/master/src/main/java/org/json/JSONObject.java#L197

Should I add another one that does a full copy?

[johnjaylward]

thinking the constructor would look like this:

    /**
     * Construct a JSONObject from another JSONObject.
     *
     * @param jo
     *            A JSONObject.
     * @throws NullPointerException
     *            If the {@code jo} JSONObject parameter is {@code null}
     */
    public JSONObject(JSONObject jo) {
        this(jo.map.size());
        this.map.putAll(jo.map);
    }

or

    /**
     * Construct a JSONObject from another JSONObject.
     *
     * @param jo
     *            A JSONObject.
     * @throws NullPointerException
     *            If the {@code jo} JSONObject parameter is {@code null}
     */
    public JSONObject(JSONObject jo) {
        this.map = new HashMap<String, Object>(jo.map);
    }

[johnjaylward]

I just noticed the JSONObject.wrap method call here (and in the other addAll method), and I think we may have an issue with consistency. If you notice both the JSONObject.put(String, Object) and JSONArray.put(Object) methods do not do any "wrap" calls to the values. However, the constructors (before the new addAll methods) both wrap values. I feel like this may have been an oversight at some point. Changing the wrap to happen consistently in the put methods breaks a number of tests.

@stleary some guidance on what to do in this case would be appreciated.

[johnjaylward]

so, to this effect, I actually worked around it in my own copy by adding the wrap calls to the put methods

[stleary]

@johnjaylward Thanks for catching these items, I added 2 new issues to track them away from this PR.

[johnjaylward]

@stleary , I changed the way the addAll and hence the putAll methods work in the latest commit. I did this so functionality of putAll would be consistent with put. You may want to reset the review timer so interested people can check it out.

[stleary]

Thanks, we still have 2 days left on the comment window, should be fine.