[
https://issues.apache.org/jira/browse/MDEP-931?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17955172#comment-17955172
]
ASF GitHub Bot commented on MDEP-931:
-------------------------------------
elharo commented on code in PR #530:
URL:
https://github.com/apache/maven-dependency-plugin/pull/530#discussion_r2115563157
##########
src/main/java/org/apache/maven/plugins/dependency/tree/DOTDependencyNodeVisitor.java:
##########
@@ -47,29 +50,45 @@ public DOTDependencyNodeVisitor(Writer writer) {
*/
@Override
public boolean visit(DependencyNode node) {
- if (node.getParent() == null || node.getParent() == node) {
- writer.write("digraph \"" + node.toNodeString() + "\" { " +
System.lineSeparator());
- }
+ try {
+ StringWriter stringWriter = new StringWriter();
+ if (node.getParent() == null || node.getParent() == node) {
+ writer.write("digraph \"" + node.toNodeString() + "\" { " +
System.lineSeparator());
+ }
- // Generate "currentNode -> Child" lines
+ // Generate "currentNode -> Child" lines
- List<DependencyNode> children = node.getChildren();
+ List<DependencyNode> children = node.getChildren();
+ for (DependencyNode child : children) {
+ stringWriter.write("\t\"" + node.toNodeString() + "\" -> \"" +
child.toNodeString() + "\" ;\n");
+ }
- for (DependencyNode child : children) {
- writer.println("\t\"" + node.toNodeString() + "\" -> \"" +
child.toNodeString() + "\" ; ");
+ // Write the accumulated output to the provided writer
+ writer.write(stringWriter.toString());
+ writer.flush();
+ return true;
+ } catch (IOException e) {
+ throw new UncheckedIOException("Failed to write to DOT output", e);
}
-
- return true;
}
/**
* {@inheritDoc}
*/
@Override
public boolean endVisit(DependencyNode node) {
- if (node.getParent() == null || node.getParent() == node) {
- writer.write(" } ");
+ try {
+ StringWriter stringWriter = new StringWriter();
Review Comment:
you don't need the stringwriter here. Just use the writer directly
##########
src/main/java/org/apache/maven/plugins/dependency/tree/JsonDependencyNodeVisitor.java:
##########
@@ -110,8 +119,16 @@ private void writeChildren(int indent, DependencyNode
node, StringBuilder sb, Se
@Override
public boolean endVisit(DependencyNode node) {
- return true;
+ try {
+ if (node.getParent() == null || node.getParent() == node) {
+ writer.flush();
Review Comment:
possibly here and only here content should be written to the stream; I'm not
sure
##########
src/main/java/org/apache/maven/plugins/dependency/tree/DOTDependencyNodeVisitor.java:
##########
@@ -47,29 +50,45 @@ public DOTDependencyNodeVisitor(Writer writer) {
*/
@Override
public boolean visit(DependencyNode node) {
- if (node.getParent() == null || node.getParent() == node) {
- writer.write("digraph \"" + node.toNodeString() + "\" { " +
System.lineSeparator());
- }
+ try {
+ StringWriter stringWriter = new StringWriter();
+ if (node.getParent() == null || node.getParent() == node) {
+ writer.write("digraph \"" + node.toNodeString() + "\" { " +
System.lineSeparator());
+ }
- // Generate "currentNode -> Child" lines
+ // Generate "currentNode -> Child" lines
- List<DependencyNode> children = node.getChildren();
+ List<DependencyNode> children = node.getChildren();
+ for (DependencyNode child : children) {
+ stringWriter.write("\t\"" + node.toNodeString() + "\" -> \"" +
child.toNodeString() + "\" ;\n");
+ }
- for (DependencyNode child : children) {
- writer.println("\t\"" + node.toNodeString() + "\" -> \"" +
child.toNodeString() + "\" ; ");
+ // Write the accumulated output to the provided writer
+ writer.write(stringWriter.toString());
+ writer.flush();
+ return true;
+ } catch (IOException e) {
+ throw new UncheckedIOException("Failed to write to DOT output", e);
Review Comment:
probably better than what we had before, but really we need a complete
rethink of this API that separates real I/O from string bullding and lets it
throw exceptions
##########
src/main/java/org/apache/maven/plugins/dependency/tree/DOTDependencyNodeVisitor.java:
##########
@@ -47,29 +50,45 @@ public DOTDependencyNodeVisitor(Writer writer) {
*/
@Override
public boolean visit(DependencyNode node) {
- if (node.getParent() == null || node.getParent() == node) {
- writer.write("digraph \"" + node.toNodeString() + "\" { " +
System.lineSeparator());
- }
+ try {
+ StringWriter stringWriter = new StringWriter();
+ if (node.getParent() == null || node.getParent() == node) {
+ writer.write("digraph \"" + node.toNodeString() + "\" { " +
System.lineSeparator());
+ }
- // Generate "currentNode -> Child" lines
+ // Generate "currentNode -> Child" lines
- List<DependencyNode> children = node.getChildren();
+ List<DependencyNode> children = node.getChildren();
+ for (DependencyNode child : children) {
+ stringWriter.write("\t\"" + node.toNodeString() + "\" -> \"" +
child.toNodeString() + "\" ;\n");
+ }
- for (DependencyNode child : children) {
- writer.println("\t\"" + node.toNodeString() + "\" -> \"" +
child.toNodeString() + "\" ; ");
+ // Write the accumulated output to the provided writer
+ writer.write(stringWriter.toString());
Review Comment:
This shouldn't be happening in the visit method. The visitor should be
building the report in memory that is only later written to a stream.
Alternately the API should change so that visit can throw an IOException.
##########
src/main/java/org/apache/maven/plugins/dependency/tree/GraphmlDependencyNodeVisitor.java:
##########
@@ -63,37 +66,55 @@ public GraphmlDependencyNodeVisitor(Writer writer) {
* {@inheritDoc}
*/
@Override
- public boolean endVisit(DependencyNode node) {
- if (node.getParent() == null || node.getParent() == node) {
- writer.write(GRAPHML_FOOTER);
- } else {
- DependencyNode p = node.getParent();
- writer.print("<edge source=\"" + generateId(p) + "\" target=\"" +
generateId(node) + "\">");
- if (node.getArtifact().getScope() != null) {
- // add Edge label
- writer.print("<data key=\"d1\"><y:PolyLineEdge><y:EdgeLabel>"
- + node.getArtifact().getScope() +
"</y:EdgeLabel></y:PolyLineEdge></data>");
+ public boolean visit(DependencyNode node) {
+ try {
+ StringWriter stringWriter = new StringWriter();
+ if (node.getParent() == null || node.getParent() == node) {
+ stringWriter.write(GRAPHML_HEADER);
}
- writer.println("</edge>");
+ // write node
+ stringWriter.write("<node id=\"" + generateId(node) + "\">");
+ // add node label
+ stringWriter.write("<data key=\"d0\"><y:ShapeNode><y:NodeLabel>" +
node.toNodeString()
+ + "</y:NodeLabel></y:ShapeNode></data>");
+ stringWriter.write("</node>\n");
+
+ // Write the accumulated output to the provided writer
+ writer.write(stringWriter.toString());
+ writer.flush();
+ return true;
+ } catch (IOException e) {
+ throw new UncheckedIOException("Failed to write GraphML node", e);
Review Comment:
I'm starting to see how this should work. Either drop all the stringwiters
and write straight onto the writer, or use stringwriters to build up a report
that is written after the visitor is finished.
##########
src/main/java/org/apache/maven/plugins/dependency/tree/JsonDependencyNodeVisitor.java:
##########
@@ -56,14 +58,20 @@ public boolean visit(DependencyNode node) {
* @param node the node to write
*/
private void writeRootNode(DependencyNode node) {
- Set<DependencyNode> visited = new HashSet<>();
- int indent = 2;
- StringBuilder sb = new StringBuilder();
- sb.append("{").append("\n");
- writeNode(indent, node, sb, visited);
- sb.append("}").append("\n");
- writer.write(sb.toString());
+ try {
+ StringBuilder sb = new StringBuilder();
+ Set<DependencyNode> visited = new HashSet<>();
+ int indent = 2;
+ sb.append("{").append("\n");
+ writeNode(indent, node, sb, visited);
+ sb.append("}").append("\n");
+ writer.write(sb.toString());
+ writer.flush();
+ } catch (IOException e) {
+ throw new UncheckedIOException("Failed to write JSON output", e);
Review Comment:
not needed; this method can and should throw the IOException directly since
it's private
> AbstractSerializingVisitor should not use PrintWriter
> -----------------------------------------------------
>
> Key: MDEP-931
> URL: https://issues.apache.org/jira/browse/MDEP-931
> Project: Maven Dependency Plugin (Moved to GitHub Issues)
> Issue Type: Improvement
> Reporter: Elliotte Rusty Harold
> Priority: Minor
>
> See https://github.com/apache/maven-dependency-plugin/pull/391/files
> AbstractSerializingVisitor uses a PrintWriter which swallows exceptions. It
> should use a regular Writer instead.
> This is exposed in the protected API so fixing this is ugly.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
