elharo commented on code in PR #1099:
URL: https://github.com/apache/maven/pull/1099#discussion_r1233935475
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -422,6 +439,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;
Review Comment:
This confused since I initially thought these were different representations
of the same value. How about stringPart and digitPart?
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -162,21 +173,39 @@ 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");
checkVersionsEqual("1Cr", "1Rc");
checkVersionsEqual("1cR", "1rC");
checkVersionsEqual("1m3", "1Milestone3");
checkVersionsEqual("1m3", "1MileStone3");
checkVersionsEqual("1m3", "1MILESTONE3");
}
+ @Test
+ void testVersionsEqualOrderAndObjectInequality() {
Review Comment:
testVersionsHaveSameOrderButAreNotEqual
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -101,6 +102,21 @@ private void checkVersionsEqual(String v1, String v2) {
assertEquals(c2, c1, "expected " + v2 + ".equals( " + v1 + " )");
}
+ private void checkVersionsEqualOrder(String v1, String v2) {
+ ComparableVersion c1 = new ComparableVersion(v1);
+ ComparableVersion c2 = new ComparableVersion(v2);
+ assertEquals(0, c1.compareTo(c2), "expected " + v1 + " == " + v2);
+ assertEquals(0, c2.compareTo(c1), "expected " + v2 + " == " + v1);
+ }
+
+ private void checkVersionObjectInequality(String v1, String v2) {
Review Comment:
checkVersionsAreNotEqual
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -383,4 +412,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";
Review Comment:
Make these variables ComparableVersions, not strings, and then inline
checkVersionsOrder in this method so it's easier to understand in one place
what is being tested. `checkVersionsOrder(f, sp1)` is unclear because it does
not indicate whether f is expected to be less than, equal to, or greater than
sp1.
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -101,6 +101,13 @@ private void checkVersionsEqual(String v1, String v2) {
assertEquals(c2, c1, "expected " + v2 + ".equals( " + v1 + " )");
}
+ private void checkVersionsEqualOrder(String v1, String v2) {
Review Comment:
I see what's happening here now, but only because I've been over this a few
times. Consider renaming this method to checkVersionsHaveSameOrder or
checkVersionsAreOrderedTheSame
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -162,21 +173,39 @@ 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");
checkVersionsEqual("1Cr", "1Rc");
checkVersionsEqual("1cR", "1rC");
checkVersionsEqual("1m3", "1Milestone3");
checkVersionsEqual("1m3", "1MileStone3");
checkVersionsEqual("1m3", "1MILESTONE3");
}
+ @Test
+ void testVersionsEqualOrderAndObjectInequality() {
+ checkVersionsEqualOrder("1ga", "1");
+ checkVersionObjectInequality("1ga", "1");
Review Comment:
Is inequality part of the API for ComparableVersion or is it an
implementation detail? If the former it should be added to the class JavaDoc.
If the latter, it shouldn't be tested.
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -688,7 +828,7 @@ public int hashCode() {
* </pre>
*
* @param args the version strings to parse and compare. You can pass
arbitrary number of version strings and always
- * two adjacent will be compared
+ * two adjacent will be compared
Review Comment:
nit: period at end
--
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]
