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

Implemented custom duplicate key handling (#840) #857

Merged
merged 7 commits into from
Mar 9, 2024

Conversation

XIAYM-gh
Copy link
Contributor

@XIAYM-gh XIAYM-gh commented Feb 13, 2024

Supports: throw a JSONException (by default), ignore (not overwritting), overwrite or merge into a JSONArray;
also with tests in a separated file

xD, thanks for reviewing

 - Supports: throw an exception (by default), ignore, overwrite & merge into a JSONArray
 - With tests, 4/4 passed.
@XIAYM-gh
Copy link
Contributor Author

whoa, 2 different errors, working on them

@stleary
Copy link
Owner

stleary commented Feb 13, 2024

@XIAYM-gh Please discuss the proposed changes in issue #840 before submitting a PR. At this time only throw would be allowed unless you can make a case for the other actions.

@XIAYM-gh
Copy link
Contributor Author

I can't believe it throws a StackOverFlowError while compiling, should I add a -Xss4M jvm argument to the actions file?

@stleary
Copy link
Owner

stleary commented Feb 15, 2024

I can't believe it throws a StackOverFlowError while compiling, should I add a -Xss4M jvm argument to the actions file?

It appears to be an intermittent error, only seems to affect Java v11, root cause unknown. After a couple of retries it was successful. Also, the build and test using Java v11 is successful on my laptop. No changes needed at this time.

However, your code is out of date, please merge from the latest master branch.

@XIAYM-gh
Copy link
Contributor Author

XIAYM-gh commented Feb 19, 2024

I found the isKeepStrings() method is unused in the JSONParserConfiguration class, should I overwrite the method and throws an UnsupportedOperationException?

@stleary
Copy link
Owner

stleary commented Feb 19, 2024

I found the isKeepStrings() method is unused in the JSONParserConfiguration class, should I overwrite the method and throws an UnsupportedOperationException?

That method is used by JSONML and XML classes, please don't make any changes to it.

@XIAYM-gh
Copy link
Contributor Author

I see, thanks!

@stleary
Copy link
Owner

stleary commented Feb 23, 2024

What problem does this code solve?
Adds an opt-in ability to overwrite a JSONObject duplicate key instead of throwing an exception

Does the code still compile with Java6?
Yes

Risks
Low

Changes to the API?
Yes, new parsing options were added

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
No, new unit tests were added

Was any code refactored in this commit?
No

Review status
APPROVED

Starting 3-day comment window

@stleary
Copy link
Owner

stleary commented Mar 3, 2024

@XIAYM-gh Please merge from the latest master branch, the PR is not up to date.

@XIAYM-gh
Copy link
Contributor Author

XIAYM-gh commented Mar 9, 2024

Oh, I apologize for the late reply, I was just back from school, and I'm sorry for the inconvenience that I've caused

Copy link
Owner

@stleary stleary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new JSONParserConfiguration ctor will be removed in a followup commit.

@stleary stleary merged commit 712859d into stleary:master Mar 9, 2024
7 checks passed
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

2 participants