[
https://issues.apache.org/jira/browse/MNG-7714?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17734146#comment-17734146
]
ASF GitHub Bot commented on MNG-7714:
-------------------------------------
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
> sp < final
> ----------
>
> Key: MNG-7714
> URL: https://issues.apache.org/jira/browse/MNG-7714
> Project: Maven
> Issue Type: Bug
> Reporter: Elliotte Rusty Harold
> Assignee: Elliotte Rusty Harold
> Priority: Major
> Fix For: 4.0.0-alpha-6
>
>
> Ported from a comment on https://issues.apache.org/jira/browse/MNG-7701
> The claim is that sp < final, which if true is incorrect according to spec.
> It is easy to demonstrate that this is not fixed and also not in line with
> the spec, with just this one important example (yes this does break for us):
> $ jbang org.apache.maven:maven-artifact:3.8.6 1.0.final-redhat-0001
> 1.0.sp1-redhat-0001
> Display parameters as parsed by Maven (in canonical form and as a list of
> tokens) and comparison result:
> 1. 1.0.final-redhat-0001 -> 1-redhat-1; tokens: [1, [redhat, [1]]]
> 1.0.final-redhat-0001 < 1.0.sp1-redhat-0001
> 2. 1.0.sp1-redhat-0001 -> 1.0.sp-1-redhat-1; tokens: [1, 0, sp, [1, [redhat,
> [1]]]]
> versus
> $ jbang org.apache.maven:maven-artifact:3.8.7 1.0.final-redhat-0001
> 1.0.sp1-redhat-0001
> Display parameters as parsed by Maven (in canonical form and as a list of
> tokens) and comparison result:
> 1. 1.0.final-redhat-0001 -> 1-redhat-1; tokens: [1, [redhat, [1]]]
> 1.0.final-redhat-0001 > 1.0.sp1-redhat-0001
> 2. 1.0.sp1-redhat-0001 -> 1-sp-1-redhat-1; tokens: [1, [sp, [1, [redhat,
> [1]]]]]
> As you can see, our `sp` release is now ordered after our `final` release
> despite this clear text in the "spec":
> Non-numeric tokens ("qualifiers") have the alphabetical order, except for
> the following tokens which come first in this order: "alpha" < "beta" <
> "milestone" < "rc" = "cr" < "snapshot" < "" = "final" = "ga" < "sp"
> It's clear that this tokenization isn't really correct by any reasonable
> measurement, and breaking large amounts of (our) existing artifacts in the
> wild is definitely not OK.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
