I tried sending an update but hit the attachment size limit. After sleeping on it I realized that making several small patches might be better from the review perspective but many decisions won't make sense until you see it in action. So I'm back to a single larger patch, but one without the advanced stuff I was experimenting with earlier. That includes taking advantage of the poorly documented (imo) fact that the DependencyNodeVisitor can know that it's the last call to endVisit() since the node will not have a parent. My patch includes this check but then does all of the real work in a different method.
This also let me check whether velocity could handle the different output formats without much effort and that's lead to a possible separate contribution to the velocity team. The class can still be duplicated in this plugin since it's not very large and it would be difficult to insist the next release depends on the most recent velocity release. However it could be marked optional since velocity is currently only used during testing.... So the changes are: - introducing a velocity util + a few related classes due to the ability/need for .dot files to include additional information - a conversion of .dot, .json, and possibly a few other formats. I had only planned to convert .dot but it looks like several other formats will also be easy to convert and will have the same improved maintainability, etc. - introducing a YamlDependencyNodeVistor. I doubt there's an outcry for it but it's an additional demonstration of how easy it is to support many formats once you have a template engine. - all changes should be limited to the 'tree' package and a new templates or velocity utils package. I looked at the other repo but didn't see an obvious place to put these changes. I guess the velocity utils could go there... One other item came up during this research - most formats include all properties, including null. This is usually fine but it makes the output files larger and that can have secondary effects. Like hitting the limit on a max attachment size. It would be easy to add a bit of logic to drop any null values but I know it can't be the default since some applications will want every value to be provided. Is it worth adding a mojo parameter to indicate whether null values should be suppressed? FWIW I'm already planning to add a mojo parameter for the name of the velocity template since users may want to customize it. The plugin will always default to the existing format (modulo possible reformatting) but it would be easy to include a second template that removes anything with a null value. I'll follow up with some links to examples on my forked repo... Bear On Sun, Mar 30, 2025 at 10:38 PM Bear Giles <bgi...@coyotesong.com> wrote: > My primary motivation is making the `mvn dependency:tree -DoutputType=dot` > output usable. That's why I've been looking at that code - I've > used 'dependency' for years. > > I can definitely look at repeating the effort but unless the existing > implementation is going away soon I would prefer to finish my current work > and then circle back to this. (This is actually about 3 levels deep from my > primary focus - it's been a series of "oh, if I have *this* then I can do > *that* more quickly. I would like to get back to those other tasks asap.) > > I'm having trouble creating a PR so I can point you to what I have as a > start. (I suspect it's because I added an 'examples' directory so I can > include links instead of attachments.) > > The fork is at https://github.com:/coyotesong/maven-dependency-plugin. > > This directory contains the results of running 'mvn dependency:tree' on > this project. > https://github.com/coyotesong/maven-dependency-plugin/tree/master/examples. > The generated image files are located in > > https://github.com/coyotesong/maven-dependency-plugin/tree/master/examples/dot/3.8.2-SNAPSHOT > . > > The proposed changes are pretty much all in this package: > https://github.com/coyotesong/maven-dependency-plugin/tree/add-velocity-utils/src/main/java/org/apache/maven/plugins/dependency/utils/templates > > There's also the test class, some test templates, etc. > > Bear > > > > On Sun, Mar 30, 2025 at 1:57 PM Guillaume Nodet <gno...@apache.org> wrote: > >> Which project are you targeting exactly ? >> If you're targeting Maven Core, do you think you could target the new API >> rather than the resolver one ? >> >> >> https://github.com/apache/maven/blob/master/api/maven-api-core/src/main/java/org/apache/maven/api/Node.java >> Or are you targeting the resolver ? >> >> Le dim. 30 mars 2025 à 19:04, Bear Giles <bgi...@coyotesong.com> a écrit >> : >> >> > I've taken the time to look around the other 'tree' classes, or more >> > precisely the '*DependencyNodeVisitor" classes, and noticed that most if >> > not all of them follow the anti-pattern of doing more than one thing in >> > each method. More precisely they mix logic ('what to include') and >> > presentation ('what to send to the writer). This makes code difficult to >> > understand, to test, to extend by a third party, etc. >> > >> > This is why template engines were developed! :-) >> > >> > Fortunately there's already a dependency on 'velocity', although it >> appears >> > to only be used during the extensive testing. However it's still there >> in >> > the 'compile' scope so using it won't require adding new dependencies. >> > >> > That's a very clean solution to the problem identified above. For most >> > formats the velocity template won't need anything beyond what's already >> > included in the WrappedNode (DependencyNode). If a format can benefit >> from >> > adding a tiny bit of additional information it can be added to the >> > WrappedNode since that class is exclusive to the 'tree' package. In more >> > complex cases the individual class can maintain it's own data structures >> > and add them to the Context provided to velocity. >> > >> > This should work with all of the existing classes although I'm only >> > planning to modify one at this time. It should make the classes much >> easier >> > to maintain since they can focus on the logic of how to convert the data >> > provided by the DependencyNode to the data usable by the Velocity >> engine. I >> > know other template engines, e.g., jinjava, have a way to provide >> > user-defined functions but I don't think velocity does - that's >> unfortunate >> > since it will require a modest amount of pre-computation. Fortunately >> that >> > can be handled separately, immediately before calling the Velocity >> 'merge' >> > method. >> > >> > This will only require a few classes - basically just a wrapper to set >> up >> > the velocity engine. It will also require a standard place to put the >> > templates and I recommend 'templates/{format}/{name} and >> > 'templates/{format}/macros/{name}. The 'format' would be 'dot', 'json', >> > etc., and the 'macros' allows velocity macros to be kept separately from >> > the templates since they're much more likely to be reused. (Hmm... >> perhaps >> > also 'templates/macros/' for macros that are format-agnostic. >> > >> > ----------------------------- >> > >> > The second PR will probably be a bit more controversial since there's >> > multiple ways to implement it and comments in some of the 'upstream' >> > methods suggest there's already WIP that may affect how the data is >> > provided to the *DependencyNodeVisitor classes. >> > >> > At the moment these classes only include two methods - visit(node) and >> > endVisit(node). It is the responsibility of the endVisit(node) method to >> > determine that all data has been received. I think that's an obvious >> > omission but the DependencyNodeVisitor interface is defined in a >> different >> > project. It would be trivial to add to the existing implementations, and >> > java8 supports optional methods so there would be no impact on others if >> > this method is added to the interface with an empty optional method. >> > >> > That's not a big problem if your visitor is able to run in a 'streaming' >> > mode, but some formats can benefit from using a 'document' mode instead. >> > In most cases this doesn't matter since the results are not intended for >> > human consumption - they need to be a standard format accepted by other >> > applications and these formats are usually designed to work in a >> streaming >> > fashion. >> > >> > However the dot format, and potentially other, are intended for human >> > consumption. The file itself isn't human-friendly but there are widely >> > available tools to convert it into human-friendly images and even >> > interactive formats like svg. In these cases it makes much more sense >> to >> > use a document mode when creating the final document since it is often >> > appropriate to provide the same information, or at least related >> > information, in multiple places in the document. >> > >> > So there's clearly a benefit to a new method that is called immediately >> > after the final endVisit(), one that provides the complete dependency >> tree. >> > The implementations should be able to ignore visit() and endVisit(). >> (And >> > in fact they can be added to the abstract class if not the >> > DependencyNodeVisitor interface) >> > >> > The question is how to implement this, esp. when looking at other >> potential >> > enhancements to the class. >> > >> > For instance it's possible to have DependencyNodeVisitor implement >> > Consumer<DependencyNode> and the 'accept()' method calls visit(). It may >> > not be practical since many implementations will need the nesting that >> > endVisit() provides. >> > >> > Another possibility is to have DependencyNodeVisitor implement >> > Function<DependencyNode, Boolean> that's called after the final >> endVisit() >> > call. Again the idea is to simplify integration into streams but >> there's no >> > reason you couldn't just use the (object::function) notation. >> > >> > I think this ultimately comes down to what's best with future >> improvements >> > to the upstream code that creates the data sent to these classes. There >> are >> > notes that indicate the desire to shift to a streaming pattern (while >> > maintaining legacy interfaces, of course) and it would make sense to >> use a >> > consistent approach here - even if it means reformatting the data >> > immediately before calling the new method. >> > >> > For now I'll just add a new method to the abstract class that all of the >> > implementations extend but I consider it a stopgap measure since this >> > functionality would be useful to third party developers, etc. >> > >> > (Sidenote - it could even quietly replace some of the existing logic >> since >> > the final method could easily perform a recursive descent of rootNode >> and >> > call visit() and endVisit() itself. I think this could be put into the >> > DependencyNodeVisitor interface as a default method so there would be no >> > risk to removing the existing code. The primary drawback would be that >> the >> > current approach allows the implementation to bail out early if it hits >> a >> > problem while this approach would require the entire rootNode structure >> to >> > be generated first.) >> > >> > Bear >> > >> >> >> -- >> ------------------------ >> Guillaume Nodet >> >
