sultan commented on code in PR #845:
URL: https://github.com/apache/maven/pull/845#discussion_r1020917747
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -350,26 +371,27 @@ public String toString()
/**
* Represents a string in the version item list, usually a qualifier.
*/
- private static class StringItem
+ public static class StringItem
implements Item
{
- private static final List<String> QUALIFIERS =
- Arrays.asList( "alpha", "beta", "milestone", "rc", "snapshot",
"", "sp" );
+ // 'pre-release' < 'snapshot' < 'release'
+ private static final List<String> QUALIFIERS = Arrays.asList(
"snapshot", "", "sp" );
Review Comment:
the heart of the fix relies here, so better have this done right!
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -91,7 +112,7 @@
/**
* Represents a numeric item in the version item list that can be
represented with an int.
*/
- private static class IntItem
+ public static class IntItem
Review Comment:
will be treated in another PR
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -41,19 +42,34 @@
* <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, with the following exceptions:
+ * <ul>
+ * <li> 'snapshot' < '' < 'sp' </li>
+ * </ul>
+ * and alias => replacement:
+ * <ul>
+ * <li> 'a' => 'alpha' </li>
+ * <li> 'b' => 'beta' </li>
+ * <li> 'm' => 'milestone' </li>
+ * <li> 'cr' => 'rc' </li>
+ * <li> 'final' = 'ga' = 'release' => '' </li>
+ * </ul>
+ * </li>
+ * <li>
+ * Following semver rules is encouraged, and some qualifiers are discouraged:
+ * <ul>
+ * <li> The usage of 'CR' qualifier is discouraged. Use 'RC' instead. </li>
+ * <li> The usage of 'final', 'ga', 'release' qualifiers is discouraged.
Use no qualifier instead. </li>
+ * <li> The usage of 'SP' qualifier is discouraged. Use version increment
instead. </li>
Review Comment:
done on both sides (code/doc)
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -41,19 +42,34 @@
* <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, with the following exceptions:
+ * <ul>
+ * <li> 'snapshot' < '' < 'sp' </li>
+ * </ul>
+ * and alias => replacement:
+ * <ul>
+ * <li> 'a' => 'alpha' </li>
+ * <li> 'b' => 'beta' </li>
+ * <li> 'm' => 'milestone' </li>
+ * <li> 'cr' => 'rc' </li>
+ * <li> 'final' = 'ga' = 'release' => '' </li>
+ * </ul>
+ * </li>
+ * <li>
+ * Following semver rules is encouraged, and some qualifiers are discouraged:
+ * <ul>
+ * <li> The usage of 'CR' qualifier is discouraged. Use 'RC' instead. </li>
+ * <li> The usage of 'final', 'ga', 'release' qualifiers is discouraged.
Use no qualifier instead. </li>
+ * <li> The usage of 'SP' qualifier is discouraged. Use version increment
instead. </li>
+ * </ul>
+ * For other qualifiers, natural ordering is used:
+ * <ul>
+ * <li>alpha = a < beta = b < ea < milestone = m < preview < rc = cr <
snapshot < '' = final = ga = release < sp</li>
+ * </ul>
+ * </li>
+ * <li>a hyphen usually precedes a qualifier, and is always less important
than digits/number, for example
+ * 1.0.RC2 < 1.0-RC3 < 1.0.1 ; but prefer '1.0.0-RC1' over '1.0.0.RC1' </li>
* </ul>
*
* @see <a
href="https://cwiki.apache.org/confluence/display/MAVENOLD/Versioning";>"Versioning"
on Maven Wiki</a>
Review Comment:
changed to refer to the website doc
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -73,7 +89,12 @@
private ListItem items;
- private interface Item
+ public ListItem getItems()
Review Comment:
will be treated in another PR
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -41,19 +42,34 @@
* <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, with the following exceptions:
+ * <ul>
+ * <li> 'snapshot' < '' < 'sp' </li>
+ * </ul>
+ * and alias => replacement:
+ * <ul>
+ * <li> 'a' => 'alpha' </li>
+ * <li> 'b' => 'beta' </li>
+ * <li> 'm' => 'milestone' </li>
+ * <li> 'cr' => 'rc' </li>
+ * <li> 'final' = 'ga' = 'release' => '' </li>
+ * </ul>
+ * </li>
+ * <li>
+ * Following semver rules is encouraged, and some qualifiers are discouraged:
+ * <ul>
+ * <li> The usage of 'CR' qualifier is discouraged. Use 'RC' instead. </li>
+ * <li> The usage of 'final', 'ga', 'release' qualifiers is discouraged.
Use no qualifier instead. </li>
+ * <li> The usage of 'SP' qualifier is discouraged. Use version increment
instead. </li>
+ * </ul>
+ * For other qualifiers, natural ordering is used:
+ * <ul>
+ * <li>alpha = a < beta = b < ea < milestone = m < preview < rc = cr <
snapshot < '' = final = ga = release < sp</li>
Review Comment:
this is just a mere example of how the alphabetical ordering works. since
this seems to be a problem, i will remove them from the doc
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -91,7 +112,7 @@
/**
* Represents a numeric item in the version item list that can be
represented with an int.
*/
- private static class IntItem
+ public static class IntItem
Review Comment:
i will make a separate PR for this particular problem then. but the problem
still needs to be addressed ...
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -404,27 +426,31 @@ public int getType()
@Override
public boolean isNull()
{
- return ( comparableQualifier( value ).compareTo(
RELEASE_VERSION_INDEX ) == 0 );
+ return QUALIFIERS.indexOf( value ) == RELEASE_VERSION_INDEX;
}
/**
- * Returns a comparable value for a qualifier.
- *
- * This method takes into account the ordering of known qualifiers
then unknown qualifiers with lexical
- * ordering.
+ * Compare the qualifiers of two artifact versions.
*
- * just returning an Integer with the index here is faster, but
requires a lot of if/then/else to check for -1
- * or QUALIFIERS.size and then resort to lexical ordering. Most
comparisons are decided by the first character,
- * so this is still fast. If more characters are needed then it
requires a lexical sort anyway.
- *
- * @param qualifier
- * @return an equivalent value that can be used with lexical comparison
+ * @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 String comparableQualifier( String qualifier )
+ public static int compareQualifiers( String qualifier1, String
qualifier2 )
{
- int i = QUALIFIERS.indexOf( qualifier );
+ 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
+ return qualifier1.compareTo( qualifier2 );
+ }
- return i == -1 ? ( QUALIFIERS.size() + "-" + qualifier ) :
String.valueOf( i );
+ // 'other qualifier' < 'snapshot' < '' < 'sp'
+ return Integer.compare( i1, i2 );
}
Review Comment:
the heart of the fix relies here, so better have this done right!
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -404,27 +426,31 @@ public int getType()
@Override
public boolean isNull()
{
- return ( comparableQualifier( value ).compareTo(
RELEASE_VERSION_INDEX ) == 0 );
+ return QUALIFIERS.indexOf( value ) == RELEASE_VERSION_INDEX;
}
/**
- * Returns a comparable value for a qualifier.
- *
- * This method takes into account the ordering of known qualifiers
then unknown qualifiers with lexical
- * ordering.
+ * Compare the qualifiers of two artifact versions.
*
- * just returning an Integer with the index here is faster, but
requires a lot of if/then/else to check for -1
- * or QUALIFIERS.size and then resort to lexical ordering. Most
comparisons are decided by the first character,
- * so this is still fast. If more characters are needed then it
requires a lexical sort anyway.
- *
- * @param qualifier
- * @return an equivalent value that can be used with lexical comparison
+ * @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 String comparableQualifier( String qualifier )
+ public static int compareQualifiers( String qualifier1, String
qualifier2 )
{
- int i = QUALIFIERS.indexOf( qualifier );
+ 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
+ return qualifier1.compareTo( qualifier2 );
+ }
- return i == -1 ? ( QUALIFIERS.size() + "-" + qualifier ) :
String.valueOf( i );
+ // 'other qualifier' < 'snapshot' < '' < 'sp'
+ return Integer.compare( i1, i2 );
}
Review Comment:
any qualifier other than 'SP' are less than 'release' or ''
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -267,7 +288,7 @@ public String toString()
/**
* Represents a numeric item in the version item list.
*/
- private static class BigIntegerItem
+ public static class BigIntegerItem
Review Comment:
will be treated in another PR
--
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]
