Copilot commented on code in PR #530:
URL: https://github.com/apache/maven-ear-plugin/pull/530#discussion_r3596518050
##########
src/main/java/org/apache/maven/plugins/ear/ResourceRef.java:
##########
@@ -69,10 +69,9 @@ public class ResourceRef {
*/
public ResourceRef(String name, String type, String auth, String
lookupName) {
if (name == null || name.isEmpty()) {
- throw new IllegalArgumentException(
- RESOURCE_REF_NAME + " in " + RESOURCE_REF_NAME + " element
cannot be null.");
+ throw new IllegalArgumentException("res-ref-name in resource-ref
element cannot be null.");
} else if ((type == null || type.isEmpty()) && (auth == null ||
auth.isEmpty())) {
- throw new IllegalArgumentException(RESOURCE_TYPE + " in " +
RESOURCE_REF_NAME + " element cannot be null ");
+ throw new IllegalArgumentException("res-type in resource-ref
element cannot be null");
}
Review Comment:
The new IllegalArgumentException messages here hardcode tag names even
though this class defines constants for them, and the second message is now
misleading: the condition triggers only when *both* res-type and res-auth are
missing, but the message only mentions res-type and also drops the trailing
period used elsewhere (see EnvEntry/EjbRef). Consider restoring the
constant-based pattern and clarifying the dependency on res-auth.
##########
src/main/java/org/apache/maven/plugins/ear/ResourceRef.java:
##########
@@ -69,10 +69,9 @@ public class ResourceRef {
*/
public ResourceRef(String name, String type, String auth, String
lookupName) {
if (name == null || name.isEmpty()) {
- throw new IllegalArgumentException(
- RESOURCE_REF_NAME + " in " + RESOURCE_REF_NAME + " element
cannot be null.");
+ throw new IllegalArgumentException("res-ref-name in resource-ref
element cannot be null.");
} else if ((type == null || type.isEmpty()) && (auth == null ||
auth.isEmpty())) {
- throw new IllegalArgumentException(RESOURCE_TYPE + " in " +
RESOURCE_REF_NAME + " element cannot be null ");
+ throw new IllegalArgumentException("res-type in resource-ref
element cannot be null");
Review Comment:
This PR is described as fixing whitespace-only classifiers (#521), but it
also changes validation/error messages in ResourceRef. If these message changes
are intentional, consider moving them to a separate PR (or updating the PR
description/issue linkage) to keep the scope focused and easier to
review/release-note.
##########
src/main/java/org/apache/maven/plugins/ear/EarMojo.java:
##########
@@ -560,9 +560,12 @@ private static File buildDestinationFile(File buildDir,
String uri) {
* @return the EAR file to generate
*/
private static File getEarFile(String basedir, String finalName, String
classifier) {
- if (classifier == null) {
+ if (classifier != null) {
+ classifier = classifier.trim();
+ }
+ if (classifier == null || classifier.isEmpty()) {
classifier = "";
- } else if (classifier.trim().length() > 0 &&
!classifier.startsWith("-")) {
+ } else if (!classifier.startsWith("-")) {
classifier = "-" + classifier;
}
Review Comment:
There are existing integration tests covering classifier behavior (e.g.
EarMojoIT#testProject022), but this whitespace-only classifier regression is
not covered. Adding an IT case with classifier=" " (and asserting no spaces
in the resulting EAR filename, and expected attach vs main artifact behavior)
would help prevent regressions.
##########
src/main/java/org/apache/maven/plugins/ear/EarMojo.java:
##########
@@ -560,9 +560,12 @@ private static File buildDestinationFile(File buildDir,
String uri) {
* @return the EAR file to generate
*/
private static File getEarFile(String basedir, String finalName, String
classifier) {
- if (classifier == null) {
+ if (classifier != null) {
+ classifier = classifier.trim();
+ }
+ if (classifier == null || classifier.isEmpty()) {
Review Comment:
Trimming the classifier in getEarFile fixes the generated filename, but
whitespace-only classifiers are still treated as “present” elsewhere (e.g.
execute() attaches an artifact when classifier != null and passes the untrimmed
classifier). If the intent is to treat blank/whitespace-only the same as null
(per #521), the classifier should be normalized once (trim + null/empty ->
null) before both getEarFile(...) and projectHelper.attachArtifact(...).
--
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]
