Stellar1999 commented on PR #2029: URL: https://github.com/apache/maven/pull/2029#issuecomment-2574250755
> [MNG-8490] Combine XmlNode and XmlNodeImpl. codehaus-plexus/plexus-xml#54 > A few points need to be addressed: > > * no third party dependency in the API > * no dependency on plexus-xml, this would re-create a cyclic dependency > * use an interface + builder api, similar to other data classes in the API > * the merge code needs to be abstracted and decoupled using a service interface so that it can be used from plugins (they are only provided with the API jars) > > For the merge service, maybe using a factory using `ServiceLoader` if we want to make that generally available. However, if we only target plugins, a service accessible from the `Session` could actually be used. But we do have plugins that use `Xpp3Dom.mergeXpp3Dom` so that feature cannot be removed from the API First of all, thank you for your code review. After reflecting on your comments and considering how to implement them, I suddenly realized that if I use the ServiceLoader approach to abstract the merge functionality and expose it, it doesn't seem significantly different from not combining the implementations of XmlNode and XmlNodeImpl. Would it be worth considering not combining them? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
