Copilot commented on code in PR #565:
URL: https://github.com/apache/maven-ear-plugin/pull/565#discussion_r4360127782
##########
src/main/java/org/apache/maven/plugins/ear/AbstractEarModule.java:
##########
@@ -266,9 +267,10 @@ public String getBundleFileName() {
String outputFileNameMapping =
earExecutionContext.getOutputFileNameMapping();
bundleFileName =
MappingUtils.evaluateFileNameMapping(outputFileNameMapping, artifact);
} catch (InterpolationException e) {
- // We currently ignore this here, cause assumption is that
- // has already been happened before..
- // FIXME: Should be checked first.
+ Log log = earExecutionContext.getLog();
+ if (log != null) {
+ log.warn("Failed to evaluate file name mapping for [" +
this + "]", e);
+ }
Review Comment:
Using `this` in a log message from inside `getBundleFileName()` can be risky
if `toString()` is overridden in a way that calls back into
`getBundleFileName()` (directly or indirectly), and it also provides less
actionable debugging info than the mapping + artifact coordinates. Consider
logging the `outputFileNameMapping` value and the artifact identifier (e.g.,
`artifact.getId()`), and optionally guard string construction behind
`log.isWarnEnabled()` to avoid unnecessary concatenation.
##########
src/main/java/org/apache/maven/plugins/ear/AbstractEarModule.java:
##########
@@ -266,9 +267,10 @@ public String getBundleFileName() {
String outputFileNameMapping =
earExecutionContext.getOutputFileNameMapping();
bundleFileName =
MappingUtils.evaluateFileNameMapping(outputFileNameMapping, artifact);
} catch (InterpolationException e) {
- // We currently ignore this here, cause assumption is that
- // has already been happened before..
- // FIXME: Should be checked first.
+ Log log = earExecutionContext.getLog();
+ if (log != null) {
+ log.warn("Failed to evaluate file name mapping for [" +
this + "]", e);
+ }
}
Review Comment:
If interpolation fails, `bundleFileName` remains unset, so subsequent calls
to `getBundleFileName()` will retry evaluation and emit the same warning
repeatedly, potentially spamming logs. Consider caching the failure (e.g., a
boolean `mappingEvaluationFailed`), or setting `bundleFileName` to a safe
fallback value in the catch block so the warning is emitted at most once per
module instance.
--
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]
