elharo commented on code in PR #1099:
URL: https://github.com/apache/maven/pull/1099#discussion_r1187611327
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -199,6 +202,7 @@ public int compareTo(Item item) {
return -1;
case STRING_ITEM:
+ case COMBINATION_ITEM:
Review Comment:
include a return for every case, as relying on fallthrough is error-prone
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -422,6 +436,106 @@ public String toString() {
}
}
+ /**
+ * Represents a combination in the version item list.
+ * It is usually a combination of a string and a number, with the string
first and the number second
+ */
+ private static class CombinationItem implements Item {
+
+ StringItem stringValue;
+
+ Item digitValue;
+
+ CombinationItem(String v) {
+ int index = 0;
+ for (int i = 0; i < v.length(); i++) {
+ char c = v.charAt(i);
+ if (Character.isDigit(c)) {
Review Comment:
I don't think we want to include non-ASCII digits here.
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -422,6 +436,106 @@ public String toString() {
}
}
+ /**
+ * Represents a combination in the version item list.
+ * It is usually a combination of a string and a number, with the string
first and the number second
+ */
+ private static class CombinationItem implements Item {
+
+ StringItem stringValue;
+
+ Item digitValue;
+
+ CombinationItem(String v) {
Review Comment:
v --> value
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -619,7 +748,13 @@ public final void parseVersion(String version) {
}
private static Item parseItem(boolean isDigit, String buf) {
- if (isDigit) {
+ return parseItem(false, isDigit, buf);
+ }
+
+ private static Item parseItem(boolean isCombination, boolean isDigit,
String buf) {
Review Comment:
What is "buf"? It's not a StringBuffer. Is there a more descriptive name for
this variable?
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -162,14 +169,14 @@ void testVersionsEqual() {
checkVersionsEqual("1A", "1a");
checkVersionsEqual("1B", "1b");
checkVersionsEqual("1M", "1m");
- checkVersionsEqual("1Ga", "1");
- checkVersionsEqual("1GA", "1");
- checkVersionsEqual("1RELEASE", "1");
- checkVersionsEqual("1release", "1");
- checkVersionsEqual("1RELeaSE", "1");
- checkVersionsEqual("1Final", "1");
- checkVersionsEqual("1FinaL", "1");
- checkVersionsEqual("1FINAL", "1");
+ checkVersionsEqualOrder("1Ga", "1");
Review Comment:
These really belong in a new test method. There's too much in this test
method already.
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -383,4 +390,15 @@ void testMng7644() {
checkVersionsEqual("2.0." + x, "2.0.0." + x); // previously
ordered, now equals
}
}
+
+ @Test
+ public void testMng7714() {
+ String f = "1.0.final-redhat";
+ String sp1 = "1.0-sp1-redhat";
+ String sp2 = "1.0-sp-1-redhat";
+ String sp3 = "1.0-sp.1-redhat";
+ checkVersionsOrder(f, sp1);
Review Comment:
This is unclear since there's nothing in the method call to say which is
expected to be greater. If these can be tested directly, it would be easier to
follow. That is, show the comparisons between f and sp1, etc.
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -147,10 +154,10 @@ void testVersionsEqual() {
checkVersionsEqual("1.0.0x", "1-x");
// aliases
- checkVersionsEqual("1ga", "1");
- checkVersionsEqual("1release", "1");
- checkVersionsEqual("1final", "1");
- checkVersionsEqual("1cr", "1rc");
+ checkVersionsEqualOrder("1ga", "1");
Review Comment:
I'd be more comfortable if existing tests didn't change
--
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: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
