michael-o commented on code in PR #929:
URL: https://github.com/apache/maven/pull/929#discussion_r1053437659
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -46,13 +46,16 @@ private Comparable newComparable(String version) {
}
private static final String[] VERSIONS_QUALIFIER = {
+ "1-abc",
Review Comment:
So this one will be interpreted as alpha?
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -40,22 +41,39 @@
* <code>1.0alpha1 => [1, 0, alpha, 1]</code></li>
* <li>unlimited number of version components,</li>
* <li>version components in the text can be digits or strings,</li>
- * <li>strings are checked for well-known qualifiers and the qualifier
ordering is used for version ordering.
- * Well-known qualifiers (case insensitive) are:<ul>
- * <li><code>alpha</code> or <code>a</code></li>
- * <li><code>beta</code> or <code>b</code></li>
- * <li><code>milestone</code> or <code>m</code></li>
- * <li><code>rc</code> or <code>cr</code></li>
- * <li><code>snapshot</code></li>
- * <li><code>(the empty string)</code> or <code>ga</code> or
<code>final</code></li>
- * <li><code>sp</code></li>
- * </ul>
- * Unknown qualifiers are considered after known qualifiers, with lexical
order (always case insensitive),
- * </li>
- * <li>a hyphen usually precedes a qualifier, and is always less important
than something preceded with a dot.</li>
+ * <li>
+ * String qualifiers are ordered lexically (case insensitive), with the
following exceptions:
+ * <ul>
+ * <li> 'snapshot' < '' < 'sp' </li>
+ * </ul>
+ * and alias -> replacement (all case insensitive):
+ * <ul>
+ * <li> 'a' -> 'alpha' </li>
+ * <li> 'b' -> 'beta' </li>
+ * <li> 'm' -> 'milestone' </li>
+ * <li> 'cr' -> 'rc' </li>
+ * <li> 'final' -> '' </li>
+ * <li> 'final' -> '' </li>
+ * <li> 'final' -> '' </li>
+ * </ul>
Review Comment:
Does this format really add a benefit here, except for the exception
documentation?
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -346,4 +342,22 @@ public void testReuse() {
assertEquals(c1, c2, "reused instance should be equivalent to new
instance");
}
+
+ /**
+ * Test <a
href="https://issues.apache.org/jira/browse/MNG-7559";>MNG-7559</a> edge cases
+ * 1.0.0.RC1 < 1.0.0-RC2
+ * -pfd < final, ga, release
+ * 2.0.1.MR < 2.0.1
+ * 9.4.1.jre16 > 9.4.1.jre16-preview
+ */
+ @Test
+ public void testMng7559() {
+ checkVersionsOrder("1.0.0.RC1", "1.0.0-RC2");
+ checkVersionsOrder("4.0.0.Beta3", "4.0.0-RC2");
+ checkVersionsOrder("2.3-pfd", "2.3");
+ checkVersionsOrder("2.0.1.MR", "2.0.1");
+ checkVersionsOrder("9.4.1.jre16-preview", "9.4.1.jre16");
+ checkVersionsEqual("2.0.a", "2.0.0.a"); // previously ordered, now
equals
+ checkVersionsOrder("1-sp-1", "1-ga-1"); // proving website
documentation right.
Review Comment:
Also there is no change here, this is weird, isn't? I mean a service pack
comes *after* `GA`, no?
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -365,18 +383,42 @@ public boolean isNull() {
*
* @param qualifier
* @return an equivalent value that can be used with lexical comparison
+ * @deprecated Use {@link #compareQualifiers(String, String)} instead
*/
+ @Deprecated
public static String comparableQualifier(String qualifier) {
- int i = QUALIFIERS.indexOf(qualifier);
+ int index = QUALIFIERS.indexOf(qualifier) + 1;
- return i == -1 ? (QUALIFIERS.size() + "-" + qualifier) :
String.valueOf(i);
+ return index == 0 ? ("0-" + qualifier) : String.valueOf(index);
+ }
+
+ /**
+ * Compare the qualifiers of two artifact versions.
+ *
+ * @param qualifier1 qualifier of first artifact
+ * @param qualifier2 qualifier of second artifact
+ * @return a negative integer, zero, or a positive integer as the
first argument is less than, equal to, or
+ * greater than the second
+ */
+ public static int compareQualifiers(String qualifier1, String
qualifier2) {
+ int i1 = QUALIFIERS.indexOf(qualifier1);
+ int i2 = QUALIFIERS.indexOf(qualifier2);
+
+ // if both pre-release, then use natural lexical ordering
+ if (i1 == -1 && i2 == -1) {
+ // alpha < beta < ea < milestone < preview < rc
Review Comment:
Wait a second, where is `ea` documented?
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -304,23 +323,22 @@ public String toString() {
* Represents a string in the version item list, usually a qualifier.
*/
private static class StringItem implements Item {
- private static final List<String> QUALIFIERS =
- Arrays.asList("alpha", "beta", "milestone", "rc", "snapshot",
"", "sp");
+ private static final List<String> QUALIFIERS =
Arrays.asList("snapshot", "", "sp");
Review Comment:
So this change relies on the lexicographical order or the tokens, right?
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -346,4 +342,22 @@ public void testReuse() {
assertEquals(c1, c2, "reused instance should be equivalent to new
instance");
}
+
+ /**
+ * Test <a
href="https://issues.apache.org/jira/browse/MNG-7559";>MNG-7559</a> edge cases
+ * 1.0.0.RC1 < 1.0.0-RC2
+ * -pfd < final, ga, release
+ * 2.0.1.MR < 2.0.1
+ * 9.4.1.jre16 > 9.4.1.jre16-preview
+ */
+ @Test
+ public void testMng7559() {
+ checkVersionsOrder("1.0.0.RC1", "1.0.0-RC2");
+ checkVersionsOrder("4.0.0.Beta3", "4.0.0-RC2");
+ checkVersionsOrder("2.3-pfd", "2.3");
+ checkVersionsOrder("2.0.1.MR", "2.0.1");
+ checkVersionsOrder("9.4.1.jre16-preview", "9.4.1.jre16");
Review Comment:
Now we support basically two qualifiers and it is not documented?
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -346,4 +342,22 @@ public void testReuse() {
assertEquals(c1, c2, "reused instance should be equivalent to new
instance");
}
+
+ /**
+ * Test <a
href="https://issues.apache.org/jira/browse/MNG-7559";>MNG-7559</a> edge cases
+ * 1.0.0.RC1 < 1.0.0-RC2
+ * -pfd < final, ga, release
+ * 2.0.1.MR < 2.0.1
+ * 9.4.1.jre16 > 9.4.1.jre16-preview
+ */
+ @Test
+ public void testMng7559() {
+ checkVersionsOrder("1.0.0.RC1", "1.0.0-RC2");
+ checkVersionsOrder("4.0.0.Beta3", "4.0.0-RC2");
+ checkVersionsOrder("2.3-pfd", "2.3");
Review Comment:
`pfd` should be defined why it is *before* `GA`.
--
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
