[ https://issues.apache.org/jira/browse/MNG-7559?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17633753#comment-17633753 ]
ASF GitHub Bot commented on MNG-7559: ------------------------------------- elharo commented on code in PR #845: URL: https://github.com/apache/maven/pull/845#discussion_r1021450876 ########## maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java: ########## @@ -404,27 +421,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. Review Comment: nit no period ########## maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java: ########## @@ -404,27 +421,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 ) Review Comment: This is a public API breaking change, and in this case I do expect there's code outside this project depending on this method. In fact, I've probably written code like that myself. I think we need to retain the existing method with the existing behavior in addition to adding this one. > ComparableVersion vs versions with custom qualifiers > ---------------------------------------------------- > > Key: MNG-7559 > URL: https://issues.apache.org/jira/browse/MNG-7559 > Project: Maven > Issue Type: Bug > Affects Versions: 3.8.3 > Reporter: Andrzej Jarmoniuk > Priority: Major > Attachments: image-2022-10-22-18-22-11-591.png > > > Since I know that ComparableVersion was brought to Maven from > versions-maven-plugin, it turns out the bug described here: > https://github.com/mojohaus/versions-maven-plugin/issues/744 > also exists in maven, at least in 3.8.3. > According to the maven version spec, versions containing a qualifier should > be treated as less major than the same versions without the qualifier. > Currently it's only the case for a few "standard" qualifiers, e.g. "-rc*", > "-alpha", etc. > However, it looks like "2.3-pfd" is deemed less major than "2.3". > {code:java} > @Test > public void testComparableVersionWithCustomQualifier() > { > assertThat( new ComparableVersion( "2.3" ).compareTo( new > ComparableVersion( "2.3-pfd" ) ), > greaterThan( 0 ) ); > } > {code} -- This message was sent by Atlassian Jira (v8.20.10#820010)