gnodet-bot commented on code in PR #13441:
URL: https://github.com/apache/maven/pull/13441#discussion_r4249373766
##########
compat/maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -567,8 +567,8 @@ void normalize() {
if (lastItem.isNull()) {
if (i == size() - 1 || get(i + 1).getType() ==
STRING_ITEM) {
remove(i);
- } else if (get(i + 1).getType() == LIST_ITEM) {
- Item item = ((ListItem) get(i + 1)).get(0);
+ } else if (get(i + 1) instanceof ListItem list &&
!list.isEmpty()) {
Review Comment:
⚠️ **Dead code:** The `!list.isEmpty()` guard is unreachable. `normalize()`
iterates right-to-left, and the parser always places `ListItem` as the terminal
element. An empty `ListItem` at that position is already removed by the `i ==
size() - 1` branch on line 568 — no earlier `i` can ever reference an empty
`ListItem` via `get(i + 1)`.
The `instanceof` pattern-matching is a good modernization and should be
kept; just drop the empty check:
```suggestion
} else if (get(i + 1) instanceof ListItem list) {
Item item = list.get(0);
```
##########
compat/maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -686,4 +686,39 @@ public void testMng7714() {
assertTrue(f.compareTo(sp2) < 0, "expected " + f + " < " + sp2);
assertTrue(f.compareTo(sp3) < 0, "expected " + f + " < " + sp3);
}
+
+ /**
+ * Regression test for <a
href="https://issues.apache.org/jira/browse/MNG-7594";>MNG-7594</a>:
+ * ComparableVersion#getCanonical() returns incorrect results for zero
between hyphens.
Review Comment:
💡 **Misleading test name:** `testMng7594` implies this test validates the
production code fix — but these assertions already pass on `master` without the
change. Consider renaming to something like `testHyphenatedZeroCanonical` to
accurately describe what is being tested.
--
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]
