[MDEP-435] Added xml outputType to dependency tree #24
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
To improve this patch:
|
@rfscholte Thanks for the comment. |
|
Yes, that xml is closer to what I would expect. The roottag could be |
|
@rfscholte Oukej, I'll update it and add tests |
|
|
@rfscholte Is this more to your liking? |
|
As for the tests.... I have an issue as all the tree dependency tests are disabled for the moment and not quite working. My skill in java is not good enough to make it work 😿 |
|
Looks quite good, just know that a project cannot have a |
Will update the root node then |
|
I don't really understand what you mean. On master branch, running Could you please elaborate? |
As i understand it |
|
So whats the next step? It would really help me in my project to have it in official release |
|
@rfscholte Hey, do you know anyone responsible for accepting merge requests? I would love to push it to next stage... |
|
Maven is maintained by a very small group of volunteers. |
rfscholte
requested changes
Nov 22, 2019
| @@ -107,7 +107,8 @@ | |||
|
|
|||
| /** | |||
| * If specified, this parameter will cause the dependency tree to be written using the specified format. Currently | |||
| * supported format are: <code>text</code> (default), <code>dot</code>, <code>graphml</code> and <code>tgf</code>. | |||
| * supported format are: | |||
| * <code>text</code> (default), <code>dot</code>, <code>graphml</code>, <code>tgf</code> and <code>xml</code>. | |||
There was a problem hiding this comment.
let's reformat this to an unordered list
There was a problem hiding this comment.
Sorry, but i do not understand the request
There was a problem hiding this comment.
Like
<ul>
<li>text (default)</li>
<li>dot</li>
..
Or
<dd>
<dt>text</dt>
<dd>A textual tree (default)<dd/>
<dt>dot</dt>
<dd>Textual output based on the DOT graph description language</dd>
..
| { | ||
| if ( node.getParent() == null || node.getParent() == node ) | ||
| { | ||
| DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance(); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Actually using DocumentBuilder and Transformer look like quite some overhead for something simple like this. I think there are faster implementations that can directly write proper xml.
There was a problem hiding this comment.
I do not code in java, so google is my main source and i haven't found any other way. As for the XXE. It is not parsing an XML byt creating it, so it should be ok right?
There was a problem hiding this comment.
I agree with @rfscholte that this is quite a heavy approach. The PrettyPrintXMLWriter from maven-shared-utils looks promising to me. It looks promising (less code, simpler code) but I'm struggling to make the tests pass. Simply put, I don't know how verify if I did it correctly.
@S4n60w3n, if I created a pull request on your MDEP-435 branch, could you verify that the PrettyPrintXMLWriter-approach gives the same outcome as your DOM-based one? Alternatively, could you tell me how I can verify that your implementation and mine have the same outcome?
There was a problem hiding this comment.
@mthmulders I will try to make time for that, but basically if you run my implementation and yours on this repo and output it to the file. Then you can make diff. It should be the same
There was a problem hiding this comment.
I don't think this is too heavyweight, but you should set namespace aware to true on the factory.
| } | ||
| catch ( ParserConfigurationException | TransformerException e ) | ||
| { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Think of a better solution: either log with Logger or rethrow.
| * @param doc Docuemnt to use | ||
| * @param node Node to get data from | ||
| */ | ||
| private Element getNode( Document doc, DependencyNode node, Boolean root ) |
There was a problem hiding this comment.
No need to use Boolean Object, primitive is preferred here.
|
@rfscholte Thanks for the review. It is much appreciated. I`ll fix your comments |
8 tasks
elharo
reviewed
Feb 18, 2020
| @@ -107,8 +107,14 @@ | |||
|
|
|||
| /** | |||
| * If specified, this parameter will cause the dependency tree to be written using the specified format. Currently | |||
| * supported format are: <code>text</code> (default), <code>dot</code>, <code>graphml</code> and <code>tgf</code>. | |||
| * These additional formats can be plotted to image files. | |||
| * supported format are: | |||
| @@ -107,8 +107,14 @@ | |||
|
|
|||
| /** | |||
| * If specified, this parameter will cause the dependency tree to be written using the specified format. Currently | |||
There was a problem hiding this comment.
will cause --> causes (tech writing lives in the eternal present)
| /** | ||
| * Render child with its children recursively | ||
| * | ||
| * @param doc Docuemnt to use |
| /** | ||
| * Get element from node | ||
| * | ||
| * @param doc Docuemnt to use |
| * Get element from node | ||
| * | ||
| * @param doc Docuemnt to use | ||
| * @param node Node to get data from |
There was a problem hiding this comment.
Node --> node (per sun javadoc conventions)
| * @author <a href="mailto:[email protected]">Bogdan Sikora</a> | ||
| * @since 3.1.2 | ||
| */ | ||
| public class XMLDependencyNodeVisitor |
There was a problem hiding this comment.
does this class have to be public?
| { | ||
| if ( node.getParent() == null || node.getParent() == node ) | ||
| { | ||
| DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance(); |
There was a problem hiding this comment.
I don't think this is too heavyweight, but you should set namespace aware to true on the factory.
elharo
reviewed
Feb 18, 2020
|
|
||
| if ( root ) | ||
| { | ||
| element = doc.createElement( "project" ); |
There was a problem hiding this comment.
A separate method for the root rather than an if statement would reduce cyclomatic complexity
|
|
||
| if ( root ) | ||
| { | ||
| element = doc.createElement( "project" ); |
There was a problem hiding this comment.
It's weird that the syntax is sort of like a pom but not fully a POM. E.g. the namespaces are different.
There was a problem hiding this comment.
Indeed, seems like it should have an XSD as well.
elharo
requested changes
Feb 18, 2020
| handleChild( doc, child, rootElement ); | ||
| } | ||
|
|
||
| TransformerFactory transformerFactory = TransformerFactory.newInstance(); |
There was a problem hiding this comment.
The serialization could be split out into a separate method.
| * | ||
| * @param writer the writer to write to. | ||
| */ | ||
| public XMLDependencyNodeVisitor( Writer writer ) |
There was a problem hiding this comment.
Writers can't be guaranteed to handle encoding properly when the XML is serialized. This should be an output stream.
| /** | ||
| * Constructor. | ||
| * | ||
| * @param writer the writer to write to. |
There was a problem hiding this comment.
nit: no period at end of doc comment, per sun Javadoc style
|
Switching to https://maven.apache.org/shared/maven-shared-utils/apidocs/org/apache/maven/shared/utils/xml/PrettyPrintXMLWriter.html is my preferred lightweight solution |
|
I |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following this checklist to help us incorporate your
contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
[MDEP-XXX] - Fixes bug in ApproximateQuantiles,where you replace
MDEP-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
mvn -Prun-its clean verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.