Copilot commented on code in PR #1287:
URL:
https://github.com/apache/maven-site-plugin/pull/1287#discussion_r3628759264
##########
src/main/java/org/apache/maven/plugins/site/render/AbstractSiteRenderingMojo.java:
##########
@@ -296,6 +296,20 @@ protected SiteRenderingContext
createSiteRenderingContext(Locale locale)
templateProperties.putAll(attributes);
}
+ if (siteModel.getSkin() == null) {
+ File siteDescriptor = new File(siteDirectory, "site.xml");
+ throw new MojoExecutionException("No skin is declared in the site
descriptor. Since the site descriptor"
+ + " 2.0.0 no longer provides a default skin, a skin must
be declared explicitly, for example in "
+ + siteDescriptor + ":" + System.lineSeparator()
+ + " <skin>" + System.lineSeparator()
+ + " <groupId>org.apache.maven.skins</groupId>" +
System.lineSeparator()
+ + " <artifactId>maven-fluido-skin</artifactId>" +
System.lineSeparator()
+ + " <version>...</version>" + System.lineSeparator()
+ + " </skin>" + System.lineSeparator()
+ + "The skin is normally inherited from the Maven parent's
site descriptor; check that the parent"
+ + " site descriptor is resolvable if you expected it to be
inherited.");
+ }
Review Comment:
This is a user/project configuration problem (missing required
configuration), so `MojoFailureException` is typically the more appropriate
exception type than `MojoExecutionException` (which implies a plugin
execution/internal error). Consider throwing `MojoFailureException` here so
Maven reports it as a build failure caused by configuration.
##########
src/main/java/org/apache/maven/plugins/site/render/AbstractSiteRenderingMojo.java:
##########
@@ -296,6 +296,20 @@ protected SiteRenderingContext
createSiteRenderingContext(Locale locale)
templateProperties.putAll(attributes);
}
+ if (siteModel.getSkin() == null) {
+ File siteDescriptor = new File(siteDirectory, "site.xml");
+ throw new MojoExecutionException("No skin is declared in the site
descriptor. Since the site descriptor"
+ + " 2.0.0 no longer provides a default skin, a skin must
be declared explicitly, for example in "
+ + siteDescriptor + ":" + System.lineSeparator()
Review Comment:
The message hard-codes \"2.0.0\" even though the check is based solely on
`siteModel.getSkin() == null`. If this path can be hit for other model versions
(or if future versions change defaults again), the message becomes misleading.
Consider either (a) omitting the specific version reference, or (b) including
the actual detected model version/namespace in the message so the guidance
stays accurate.
##########
src/it/projects/gh-1286-missing-skin/verify.groovy:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+File buildLog = new File( basedir, 'build.log' )
+assert buildLog.exists()
+
+String log = buildLog.text
+
+// The build must fail with a clear, actionable message ...
+assert log.contains( 'No skin is declared in the site descriptor' )
+
+// ... and NOT with the previous cryptic NullPointerException.
+assert !log.contains( 'NullPointerException: skin cannot be null' )
Review Comment:
This negative assertion is tightly coupled to a specific NPE message string,
which may vary across Doxia/Maven versions or logging formats. To reduce IT
flakiness while preserving intent, consider asserting the absence of the
broader marker(s) (e.g., `NullPointerException` and/or `skin cannot be null`
separately) rather than the exact combined text.
--
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]
