gnodet-bot commented on code in PR #499:
URL:
https://github.com/apache/maven-resources-plugin/pull/499#discussion_r4102604889
##########
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 {
Review Comment:
⚠️ **Still unaddressed (4th review).** `skip` is `@Parameter(property =
"maven.test.skip")` — field injection handles `maven.test.skip`. `isTestSkip()`
also reads `maven.test.skip` from the session. The `skip ||` duplicates the
check.
With master now on rc-7 (field-injection bug fixed), the session-property
fallback is dead code. Master's approach from PR #500 (`skip || isSkip()`) is
simpler and sufficient:
```suggestion
if (skip || isSkip()) {
```
Or, if this PR's intent is to add value beyond #500, consider closing it —
#500 already merged the equivalent fix.
##########
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.
Review Comment:
⚠️ **Javadoc still inaccurate (3rd review).** `isSkip()` checks
`maven.resources.skip`, not the "inherited `skip` parameter". The local `skip`
field is bound to `maven.test.skip`. The doc conflates the two.
```suggestion
* Returns {@code true} if test resource copying should be skipped.
* Checks {@code maven.resources.skip} via the inherited {@link
#isSkip()} and reads
* {@code maven.test.skip} directly from session properties as a
fallback when
* field injection is unavailable.
```
##########
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:
⚠️ **Dead code on rc-6+.** This entire method is a workaround for the rc-5
configurator bug (apache/maven#11425) that silently failed to write private
fields. Fixed in rc-6; master is on rc-7. The session-property fallback will
never fire because `skip` is correctly injected.
Master already uses `skip || isSkip()` (PR #500) which handles both
`maven.test.skip` (via field) and `maven.resources.skip` (via `isSkip()`). This
method adds an unused `java.util.Map` import and dead code paths.
Recommend either:
- Closing this PR (superseded by #500)
- Or rebasing to remove `isTestSkip()` and revert to `skip || isSkip()` if
there are other changes worth keeping
--
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]
