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
