DRAFT: (JSONArray).toList() breaks format of entries

[ZachsCoffee]

fix for: #737
PR for discussion:

I suggest making the JSONArray class to implement the List because contains the functionality of a list also is powered by an ArrayList which is a List.
By making it a List it can be used by others like any other List class making it more useful.
Also automatically contains the stream() method that we want.

If we proceed with this approach I will add tests for the new methods.

[stleary]

Can't be accepted because of broken unit tests. It's recommended to run the unit tests locally before submitting a pull request, to catch problems like this. In this case, changing the JSONArray implements type is likely to break some existing projects that use JSON-Java.

[ZachsCoffee]

@stleary I open the PR as a draft for discussion to so my suggestion as code. I didn't give time to the tests because of this. If we proceed with this implementation then I will fix the tests also I will add test methods for the new methods.

Can you please explain to me a case that this change isn't backward compatible? Because the JSONArray was an Iterable and the List also is an Iterable.

[johnjaylward]

Instead of adding a bunch of little comments, here is a single one.

Overall I don't think these overrides should NOT be calling anything directly on myArrayList. Each of the overrides should be using the similar JSONArray.put, set, addAll, etc. methods that already exist and have been tested.

Items that are thoroughly new to the interface like subList should be implemented in a way that the sub-list return is also a valid JSONArray.

Similarly, I don't like the existing iterator implementation that just exposes the underlying collection iterator. Changes to this implementation may be out of scope for this PR, but directly exposing the underlying data may not be the expected behavior by callers. My personal preference would be that anything returned by using the iterator would be a valid JSON value (i.e. Maps would be returned as JSONObject, Double values would not return is they are NaN, etc.)

[stleary]

@ZachsCoffee Good point. In the past, PRs have only been used for an actual request to update the repo, but a draft or work-in-progress designation makes sense. I will update the Wiki at some point on how to do this. Probably something like a DRAFT: prefix and I will add a Draft label as well. You can review the CI results to see the errors, or just run it locally.

[ZachsCoffee]

Thank you @stleary. Yes, it will be good to have draft PRs. Yes, you are right, as I see from the broken tests that there is some possibility of breaking changes. But I think it's a rare case.

To fall into this case, they should have overload methods that accept Iterable and other methods List for the same argument position, and when they pass a JSONArray will have the problem. Before the changes, it will call the method with Iterable as an argument, but now it will call the method with List as argument. And this is possible to create problems, but as I said, I think this is a rare case.

@johnjaylward I see your point. The changes that you describe is certain to be breaking changes :P
You are right, if a user adds objects at the list will get the same objects back, but if from a json builds the JSONArray will get JSONObjects ...

So because the above + is getting out of scope I think it will better to have a new stream() method like the Collection interface.

[stleary]

Closing due to no activity in the past year