gnodet-bot commented on code in PR #499:
URL:
https://github.com/apache/maven-resources-plugin/pull/499#discussion_r4102521724
##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -62,17 +64,7 @@ public class TestResourcesMojo extends ResourcesMojo {
* {@inheritDoc}
*/
public void execute() throws MojoException {
- // isSkip() reads ResourcesMojo's own field. Both classes declare a
private
- // "skip", so the two collapse into a single descriptor parameter and
the
- // configurator writes the superclass one, leaving this class's field
false
- // however the build configured <skip>. Reading both is what makes
- // <skip>true</skip> reach this goal at all.
- //
- // TODO temporary: drop the isSkip() half once apache/maven#12626 is
in a
- // release. That fixes the cause in the core configurator, where
- // buildFieldCache() lets a parent field shadow the child's, and then
this
- // class's own field will be configured directly.
- if (skip || isSkip()) {
+ if (skip || isTestSkip()) {
Review Comment:
⚠️ **Still unaddressed from previous review.** `skip` is
`@Parameter(property = "maven.test.skip")` — field injection already handles
`maven.test.skip`. `isTestSkip()` also reads `maven.test.skip` from the
session. The outer `skip ||` duplicates that check.
Moreover, with master now on rc-7 (where the field-injection bug is fixed),
the session-property fallback in `isTestSkip()` is unnecessary. The simpler
`skip || isSkip()` from master (PR #500) already handles both `maven.test.skip`
(via `skip` field) and `maven.resources.skip` (via `isSkip()`).
Suggestion: consider whether this PR should be closed in favor of #500's
approach already on master, or rebased to only delete `Jenkinsfile.disable`.
##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -85,6 +77,23 @@ public void execute() throws MojoException {
super.doExecute();
}
+ /**
+ * Returns {@code true} if test resource copying should be skipped.
+ * Checks both the inherited {@code skip} parameter (bound to {@code
maven.resources.skip})
+ * and the {@code maven.test.skip} property from the session.
+ *
+ * @return {@code true} if test resources should not be copied
+ */
+ private boolean isTestSkip() {
+ if (isSkip()) {
+ return true;
+ }
+ Map<String, String> userProps = session.getUserProperties();
+ Map<String, String> sysProps = session.getSystemProperties();
+ String testSkip = userProps.getOrDefault("maven.test.skip",
sysProps.get("maven.test.skip"));
+ return Boolean.parseBoolean(testSkip);
+ }
Review Comment:
⚠️ **Unnecessary complexity given rc-7.** This entire method is a workaround
for the rc-5 configurator bug (apache/maven#11425) that silently failed to
write private fields via direct field injection. That bug was fixed in rc-6 and
master is now on rc-7.
The session-property fallback (`userProps.getOrDefault("maven.test.skip",
sysProps.get(...))`) will never fire on rc-6+ because the `skip` field is
correctly injected. This adds dead code and an unnecessary `java.util.Map`
import.
**Javadoc is also still inaccurate** (previous finding): the doc says it
checks "the inherited `skip` parameter (bound to `maven.resources.skip`)" but
`isSkip()` is what checks `maven.resources.skip` — the local `skip` field is
bound to `maven.test.skip`, not `maven.resources.skip`.
##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -62,17 +64,7 @@ public class TestResourcesMojo extends ResourcesMojo {
* {@inheritDoc}
*/
public void execute() throws MojoException {
- // isSkip() reads ResourcesMojo's own field. Both classes declare a
private
- // "skip", so the two collapse into a single descriptor parameter and
the
- // configurator writes the superclass one, leaving this class's field
false
- // however the build configured <skip>. Reading both is what makes
- // <skip>true</skip> reach this goal at all.
- //
- // TODO temporary: drop the isSkip() half once apache/maven#12626 is
in a
- // release. That fixes the cause in the core configurator, where
- // buildFieldCache() lets a parent field shadow the child's, and then
this
- // class's own field will be configured directly.
- if (skip || isSkip()) {
+ if (skip || isTestSkip()) {
Review Comment:
⚠️ **Still unaddressed from previous review.** `skip` (field injected from
`maven.test.skip`) and `isTestSkip()` (reads `maven.test.skip` from session
properties) both check the same property — the outer `skip ||` is redundant
with the session-based fallback inside `isTestSkip()`.
The cleaner approach remains: move `skip` into `isTestSkip()` so it is the
single source of truth for all skip logic:
```suggestion
if (isTestSkip()) {
```
Then update `isTestSkip()` to incorporate the field check:
```java
private boolean isTestSkip() {
if (skip || isSkip()) {
return true;
}
Map<String, String> userProps = session.getUserProperties();
Map<String, String> sysProps = session.getSystemProperties();
String testSkip = userProps.getOrDefault("maven.test.skip",
sysProps.get("maven.test.skip"));
return Boolean.parseBoolean(testSkip);
}
```
This makes the intent clear: `isTestSkip()` checks everything, and
`execute()` just calls it.
--
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]
