-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
JSONObject parsing for self-referencing maps #809
JSONObject parsing for self-referencing maps #809
Conversation
# Conflicts: # src/main/java/org/json/JSONObject.java # src/test/java/org/json/junit/JSONObjectTest.java
…ove the builder pattern and make it "with" pattern
…also remove the builder pattern and make it "with" pattern" This reverts commit 44ee3eb
…also remove the builder pattern and make it "with" pattern" This reverts commit 44ee3eb
…to fix_for_701_without_abstract
@ZachsCoffee BiConsumer is Java 8 feature. Can you find an alternate implementation that only requires Java 7? Sorry, this is a recent decision (see comments in #789 and issue #799) and has not been communicated to the team yet. |
Yes @stleary I will find an alternative, but I don't have the time today make the change. Probably tomorrow. |
@stleary is an alternative still needed if we do the point release off of the 20230618 tag? |
@johnjaylward Good point. Here is my take on it: |
Would you like a PR to set the current compatible JVM to 1.6, or do you prefer to do the point releases on the older tag though? If we downgrade the master branch back to 1.6, I think more than a few people will be disappointed. Doing the point releases on 20230618 (like a 20230618.001) which only has the security updates would be simple enough and still give a path forward for java8 in master. |
I don't think any version-specific changes are needed now, and hopefully, will never be needed. This is just a developer restriction to ensure the source code is compatible with java6 (or possibly 7?). If a user is stuck on an earlier Java version, they will just have to build manually, and won't benefit from the unit tests.
After 20 years at Cisco, I hope never to see another point release (we called them hotfixes). That was a consequence of a perceived need to maintain multiple release trains, which I think was error-prone, effort-intensive, and overall an expensive miscalculation. |
Without some kind of build restriction, I think it will be hard to ensure any kind of source compatibility. Maybe what you are suggesting is that we re-add the "build-1.6" build that was just compiling the |
For now, I don't mind doing a manual check during the review process. I believe that Gradle and Maven releases won't build with Java 6, and you can't even install the JDK on a Mac with a recent OS. |
See #815 |
@ZachsCoffee I think your branch may be out of date: From https://github.com/ZachsCoffee/JSON-java
|
@ZachsCoffee Thanks for your PR. It turns out JSONParserConfiguration was added in #823, and I am merging your unit tests into #846 so they don't get lost. Closing this PR since all of the functionality is implemented elsewhere. |
Fix for #701
Edit: without the
AbstractConfiguration
classA new configuration class was added for the JSON parser.
JSONObject
class.JSONObject
class is controlled from theJSONParserConfiguration
class. This way we can give the possibility for someone to enable the circular dependency check or if we change mid we can make the default functionality to be the check to be one. This change will not affect any other class other thanJSONParserConfiguration
.Map
) here:org.json.JSONObject#populateMap
so I left it as is. If we want to change it and respect the new configuration for the check I can make it easily.