gnodet-bot commented on code in PR #499:
URL:
https://github.com/apache/maven-resources-plugin/pull/499#discussion_r4093222837
##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -75,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 is inaccurate.** The method does NOT check `maven.test.skip`
via the injected `skip` parameter — that check is in `execute()`, outside this
method. `isTestSkip()` checks (1) `maven.resources.skip` via `isSkip()` and (2)
`maven.test.skip` via session properties. The current wording implies both
checks live inside `isTestSkip()` and is misleading to readers.
```suggestion
* Returns {@code true} if test resource copying should be skipped.
* Checks {@code maven.resources.skip} (inherited from the parent mojo)
and reads
* {@code maven.test.skip} directly from the session properties as a
fallback when
* field injection is unavailable.
```
##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -62,7 +64,7 @@ public class TestResourcesMojo extends ResourcesMojo {
* {@inheritDoc}
*/
public void execute() throws MojoException {
- if (skip) {
+ if (skip || isTestSkip()) {
Review Comment:
⚠️ **Redundant check — `skip` and `isTestSkip()` both cover
`maven.test.skip`.**
The local `skip` field is `@Parameter(property = "maven.test.skip")`.
`isTestSkip()` also reads `maven.test.skip` from session properties. So this
condition checks the same property twice: once via field injection, once via
the session fallback. That's the intent (tolerate rc-5 injection failure), but
it means `isTestSkip()` is NOT a clean abstraction — it's a partial duplicate
of the outer check.
The cleaner fix is to move all skip logic into `isTestSkip()` and drop the
`skip ||` from the outer call:
```suggestion
if (isTestSkip()) {
```
Then update `isTestSkip()` to be the single source of truth:
```java
private boolean isTestSkip() {
// Check injection-based field first (prefers setter over direct field —
works on all rc versions)
if (skip) {
return true;
}
// Fallback: read maven.test.skip directly from session (tolerates
field-injection failure)
Map<String, String> userProps = session.getUserProperties();
Map<String, String> sysProps = session.getSystemProperties();
String testSkip = userProps.getOrDefault("maven.test.skip",
sysProps.get("maven.test.skip"));
if (Boolean.parseBoolean(testSkip)) {
return true;
}
// Also honour the parent's maven.resources.skip (skips all resource
processing)
return isSkip();
}
```
This makes the fallback intent explicit and removes the confusing `skip ||`
at the call site.
--
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]
