gnodet commented on PR #1205: URL: https://github.com/apache/maven/pull/1205#issuecomment-1886753042
> Mostly nits. They should be all fixed now. > More generally, rereading this I'm struck at how much work this is. You've provided a nearly complete XInclude implementation. The maintenance burden on this code is likely to be high. It feels like this belongs in the parser, or perhaps a separate library on top of the parser, and all Maven should do when reading a pom is flip the flag to turn on XInclude support. Maven's XML parsing is already quite non-standard and avoids the normal parsers like Xerces or the JDK's because of decisions made 20 years ago when the easier path was not so obvious. This is unfortunately baked into a lot of the code and even the public APIs, so I'm not sure how much of that can be repaired now. > > Nonetheless, I do wonder if this code belongs here instead of in the parser. We switched to a standard StaX parser in 4.x. The xml parser is now Woodstox, which is a fully conformant parser. Unfortunately, I don't know any StaX parser that provides support for XInclude, so I ended up writing one, borrowing the XPointer implementation from Apache Woden. I'd rather keep it separated from the parser so that we can eventually switch to Aalto which is slightly faster than Woodstox, but there were a few problems when I tried initially. As for the location, I originally put it in an external repository (https://github.com/gnodet/stax-xinclude), but you asked to move it back in this repository. I agree, this library is independent of Maven and would be better outside. I can maintain it as a personal project (and rename the package) if you think it's better, I don't really mind. -- 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
