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

Add a config flag to disable whitespace trimming #832

Merged

Conversation

keatontaylor10
Copy link
Contributor

Related issue: #695 (comment)

Notes about implementation:

  • An XMLParserConfiguration object with a boolean shouldTrimWhitespace has been added and can be passed into the toJSONObject method to disable whitespace trimming. This is as discussed in the issue.
  • Inputs with tags that are the same as the cDataTagName variable ("content" by default) are unsupported with whitespace trimming disabled. This has been documented in the javadoc comments for the new methods.
  • Tests have been added to verify the new behaviour as well as the old. A test also documents the effects of using the unsupported usage.

@keatontaylor10 keatontaylor10 marked this pull request as ready for review November 21, 2023 07:57
@stleary
Copy link
Owner

stleary commented Nov 21, 2023

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

@stleary
Copy link
Owner

stleary commented Nov 26, 2023

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

image

@keatontaylor10
Copy link
Contributor Author

@stleary I have merged and pushed to my branch

@keatontaylor10
Copy link
Contributor Author

Hi, is anything else required to get this merged?

@stleary
Copy link
Owner

stleary commented Jan 8, 2024

@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
Copy link
Contributor Author

@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
Copy link
Owner

stleary commented Jan 16, 2024

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
Copy link
Contributor Author

keatontaylor10 commented Jan 18, 2024

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
Copy link
Owner

stleary commented Jan 22, 2024

@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
Copy link
Contributor Author

@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
Copy link
Owner

stleary commented Jan 23, 2024

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

@stleary stleary merged commit f2d2098 into stleary:master Jan 27, 2024
7 checks passed
@keatontaylor10
Copy link
Contributor Author

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
Copy link
Owner

stleary commented Jan 30, 2024

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

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.

3 participants