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

Using JAXP on XMLPrinter implementation #4166

Merged
merged 12 commits into from
Oct 22, 2023
Merged

Using JAXP on XMLPrinter implementation #4166

merged 12 commits into from
Oct 22, 2023

Conversation

lcbarcellos
Copy link
Contributor

Fixes #2749

The original implementation "manually" generates XML and does not take care of special cases, when illegal characters are present on attribute values. The proposed solution avoid the "manual" generation of XML, by using JAXP API for outputing XML structure.

Also, instead of generating the complete XML string in memory, user of XMLPrinter has now the option of streaming data directly to an output file, by using public methods of the class.

Replace "manual" generation of XML by use of
JAXP API. The test cases were replaced as well
so they now check for XML document equality
instead of string equality.
@lcbarcellos
Copy link
Contributor Author

lcbarcellos commented Oct 19, 2023

What does that codecov fail mean? It seems it's not related to the code in the commits, but related to some bug in codecov. Is it possible to rerun that specific check?

}
}

public Document getDocument(String xml) throws SAXException, IOException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there no need to close the inputStream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per JavaDoc, closing a ByteArrayInputStream has no effect.

}

public StringWriter stringWriterOutput(Node node, String name) {
StringWriter stringWriter = new StringWriter();
Copy link
Collaborator

@jlerbsc jlerbsc Oct 20, 2023

Choose a reason for hiding this comment

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

I imagine that this method allows you to implement data streaming in a file, perhaps you can specify in javadoc that the resource returned by the method must be closed by the user?

@jlerbsc
Copy link
Collaborator

jlerbsc commented Oct 20, 2023

Thank you for this interesting development. There are some outstanding questions and it seems to me that it would be interesting to add some comments to clarify the intentions of certain added methods. A few additional unit test cases would also be welcome to illustrate the use of the new methods and more particularly the data streaming that you mention.

@jlerbsc
Copy link
Collaborator

jlerbsc commented Oct 20, 2023

it's not related to the code

you are right it's not related to the code.

@lcbarcellos
Copy link
Contributor Author

Thank you for this interesting development. There are some outstanding questions and it seems to me that it would be interesting to add some comments to clarify the intentions of certain added methods. A few additional unit test cases would also be welcome to illustrate the use of the new methods and more particularly the data streaming that you mention.

I've made minor changes and added documentation as well as additional tests. Also, I've made changes in small dedicated commits hoping that will make the revision easier.

lcbarcellos and others added 2 commits October 21, 2023 18:34
No need to worry about which charset is used, as the same
is going to be used for saving and loading.
@codecov
Copy link

codecov bot commented Oct 22, 2023

Codecov Report

Merging #4166 (3d0133d) into master (744bbbd) will increase coverage by 0.009%.
The diff coverage is 78.873%.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff               @@
##              master     #4166       +/-   ##
===============================================
+ Coverage     57.975%   57.985%   +0.009%     
  Complexity      2334      2334               
===============================================
  Files            645       645               
  Lines          34535     34576       +41     
  Branches        5981      5976        -5     
===============================================
+ Hits           20022     20049       +27     
- Misses         12354     12369       +15     
+ Partials        2159      2158        -1     
Flag Coverage Δ
AlsoSlowTests 57.985% <78.873%> (+0.009%) ⬆️
javaparser-core 50.522% <78.873%> (+0.023%) ⬆️
javaparser-symbol-solver 37.083% <0.000%> (-0.045%) ⬇️
jdk-10 57.973% <78.873%> (+0.009%) ⬆️
jdk-11 57.973% <78.873%> (+0.009%) ⬆️
jdk-12 57.973% <78.873%> (+0.009%) ⬆️
jdk-13 57.973% <78.873%> (+0.009%) ⬆️
jdk-14 57.967% <78.873%> (+0.003%) ⬆️
jdk-15 57.973% <78.873%> (+0.009%) ⬆️
jdk-16 57.944% <78.873%> (+0.009%) ⬆️
jdk-8 57.970% <78.873%> (+0.009%) ⬆️
jdk-9 57.973% <78.873%> (+0.012%) ⬆️
macos-latest 57.979% <78.873%> (+0.009%) ⬆️
ubuntu-latest 57.973% <78.873%> (+0.009%) ⬆️
windows-latest 57.967% <78.873%> (+0.009%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...java/com/github/javaparser/printer/XmlPrinter.java 79.268% <78.873%> (-13.415%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 744bbbd...3d0133d. Read the comment docs.

@jlerbsc jlerbsc merged commit 461cdc1 into javaparser:master Oct 22, 2023
31 of 32 checks passed
@jlerbsc jlerbsc added this to the next release milestone Oct 22, 2023
@jlerbsc jlerbsc added the PR: Changed A PR that changes implementation without changing behaviour (e.g. performance) label Oct 22, 2023
@jlerbsc
Copy link
Collaborator

jlerbsc commented Oct 22, 2023

Thank you for this contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Changed A PR that changes implementation without changing behaviour (e.g. performance)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated XML is invalid
2 participants