gnodet commented on code in PR #13722:
URL: https://github.com/apache/maven/pull/13722#discussion_r4461479951
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java:
##########
@@ -114,21 +119,131 @@ public Model read(XmlReaderRequest request) throws
IOException {
}
}
try {
- return doRead(request);
- } catch (IOException | XmlReaderException e) {
+ return doRead(source, options);
+ } catch (ModelParserException e) {
if (!exceptions.isEmpty()) {
- IOException ioException = new
IOException(buildDetailedErrorMessage(pomFile, exceptions, e));
- exceptions.values().forEach(ioException::addSuppressed);
- ioException.addSuppressed(e);
- throw ioException;
+ ModelParserException mpe =
+ new
ModelParserException(buildDetailedErrorMessage(pomFile, exceptions, e), e);
+ exceptions.values().forEach(mpe::addSuppressed);
+ mpe.addSuppressed(e);
Review Comment:
**Redundant exception in suppressed list**
`e` is set as the _cause_ of `mpe` (via the constructor), and then also
added as a _suppressed_ exception via `mpe.addSuppressed(e)`. This duplicates
it: when the exception is printed, `e` appears both as the direct cause chain
(`Caused by:`) **and** in the suppressed list (`Suppressed:`).
The original pre-refactoring code had the same pattern for `IOException`,
but that doesn't make it correct here.
```suggestion
ModelParserException mpe =
new
ModelParserException(buildDetailedErrorMessage(pomFile, exceptions, e), e);
exceptions.values().forEach(mpe::addSuppressed);
throw mpe;
```
> [!NOTE]
> *Posted by [Hermes](https://github.com/gnodet/hermes-review) on behalf of
@gnodet* 🤖
##########
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/ModelProcessor.java:
##########
@@ -34,6 +37,15 @@
*/
public interface ModelProcessor {
+ /**
+ * Option key for passing an {@link XmlReaderRequest.Transformer} through
the options map.
+ * When present, the processor should use this transformer for XML parsing
instead
Review Comment:
**SPI Javadoc leaks XML-specific type**
The Javadoc for the `TRANSFORMER` constant references `{@link
XmlReaderRequest.Transformer}`, which is an XML-specific type
(`maven-api-core/services/xml`). The goal of this PR (#13707) is to decouple
`ModelProcessor` from XML specifics — this Javadoc directly contradicts that
goal and would still couple implementors to the XML layer conceptually.
Consider describing the transformer contract in format-agnostic terms, e.g.:
```suggestion
/**
* Option key for passing a source transformer through the options map.
* When present, the processor may use this transformer to pre-process
* the input before parsing.
*
* @since 4.1.0
*/
```
Or, if the intent is that only the XML implementation uses this key, clarify
that in the Javadoc and add a `@see` to `XmlReaderRequest.Transformer` without
making it the primary type reference.
> [!NOTE]
> *Posted by [Hermes](https://github.com/gnodet/hermes-review) on behalf of
@gnodet* 🤖
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
