stleary / JSON-java

A reference implementation of a JSON package in Java.
http://stleary.github.io/JSON-java/index.html
Other
4.54k stars 2.56k forks source link

Add a config flag to disable whitespace trimming #832

Closed keatontaylor10 closed 10 months ago

keatontaylor10 commented 1 year ago

Related issue: https://github.com/stleary/JSON-java/issues/695#issuecomment-1815041209

Notes about implementation:

stleary commented 1 year ago

@keatontaylor10 Thanks for the PR and the careful work. Code review is in progress, but have patience due to the holiday.

stleary commented 12 months ago

@keatontaylor10 Out of sync, please merge master to your branch.

image

keatontaylor10 commented 12 months ago

@stleary I have merged and pushed to my branch

keatontaylor10 commented 10 months ago

Hi, is anything else required to get this merged?

stleary commented 10 months ago

@keatontaylor10 Please respond to this question: "Setting a global flag changes the processing of the entire doc. Maybe it would be better to restrict the whitespace flag to specified tags, similar to how forceList works. Could you make that work for your project?"

keatontaylor10 commented 10 months ago

@keatontaylor10 Please respond to this question: "Setting a global flag changes the processing of the entire doc. Maybe it would be better to restrict the whitespace flag to specified tags, similar to how forceList works. Could you make that work for your project?"

For our use case this would not work unfortunately as we don't know what all the tags will be at this point and we need to disable whitespace trimming on all the tags. Would this feature be required to be set per tag?

stleary commented 10 months ago

Hi, is anything else required to get this merged?

@keatontaylor10 I just resolved all but a couple of the comments. I think there are several comments you have not responded to yet regarding CDATA and empty arrays. If you are not sure of a response, no worries, I will add some tests to understand the behavior.

Please do one more merge, it looks like the PR has gone out of sync again.

Thanks for being patient, progress is being made.

keatontaylor10 commented 10 months ago

I have synced my branch. I also left a couple more comments to hopefully answer your questions in a bit more detail. Thanks for taking the time to review!

stleary commented 10 months ago

@keatontaylor10 My testing did not raise any red flags. However, there is an edge case that's worth noting. Are you sure you want this behavior? If you are OK with it, I can go ahead and approve the PR.

image

Here is a slightly different variation that illustrates CDATA handling can give similar unexpected results even for the legacy code. I guess I would consider this a newly discovered bug in the existing code.

image

keatontaylor10 commented 10 months ago

@stleary Thanks for testing. This behaviour is fine for my use case as our inputs won't have CDATA tags so we won't run into this problem.

stleary commented 10 months ago

What problem does this code solve? Opt-in feature to retain whitespace in XML content when converting to JSON

Does the code still compile with Java6? Yes

Risks Low

Changes to the API? A new flag is available in XMLParserConfiguration

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

keatontaylor10 commented 9 months ago

Thank you for reviewing and getting this merged. @stleary what would it take to get these commits built into a new release as that is required for me to pull this into my project, thanks.

stleary commented 9 months ago

@keatontaylor10 it's a little early for the next release, but I will try for this weekend.