Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MNG-7559] Fix versions comparison #845

Merged
merged 1 commit into from
Dec 3, 2022
Merged

[MNG-7559] Fix versions comparison #845

merged 1 commit into from
Dec 3, 2022

Conversation

sultan
Copy link
Contributor

@sultan sultan commented Oct 22, 2022 •
edited
Loading

Fix versions comparison https://issues.apache.org/jira/browse/MNG-7559
intention is:

  • 1.0.0.RC1 < 1.0.0-RC2
  • ( edr, pfd, etc.) < final, ga, release
  • 9.4.1.jre16 > 9.4.1.jre16-preview

following semver rules should be encouraged, natural ordering is used without the need to hard code strings, except for hard coded qualifiers 'a', 'b', 'm', 'cr', 'snapshot', 'final', 'ga', 'release', '' and 'sp':

  • alpha = a < beta = b < milestone = m < rc = cr < 'snapshot' < '' = 'final' = 'ga' = 'release' < 'sp'

the documentation should discourage the usage of 'CR', 'final', 'ga', 'release' and 'SP' qualifiers.
Maven Central should begin to reject new artifact using CR and SP qualifiers.

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [MNG-XXX] SUMMARY, where you replace MNG-XXX
    and SUMMARY with the appropriate JIRA issue. Best practice is to use the JIRA issue
    title in the pull request title and in the first line of the commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@sultan sultan changed the title Fix versions comparison |MNG-7559] Fix versions comparison Oct 22, 2022
@sultan sultan changed the title |MNG-7559] Fix versions comparison [MNG-7559] Fix versions comparison Oct 22, 2022
@sultan sultan force-pushed the comparison branch 2 times, most recently from 194316a to 9372f04 Compare October 22, 2022 18:35
@sultan sultan marked this pull request as ready for review November 6, 2022 11:35
@sultan sultan mentioned this pull request Nov 6, 2022
Closed
@sultan
Copy link
Contributor Author

sultan commented Nov 6, 2022 •
edited
Loading

made the items attribute private with a public getter to respect the code standard

@sultan sultan marked this pull request as draft November 6, 2022 12:28
@sultan sultan force-pushed the comparison branch 2 times, most recently from 83a0828 to 8e382dd Compare November 6, 2022 14:42
@sultan
Copy link
Contributor Author

sultan commented Nov 6, 2022

added regression test cases

@sultan sultan marked this pull request as ready for review November 6, 2022 14:43
@sultan sultan marked this pull request as draft November 6, 2022 15:12
@sultan sultan marked this pull request as ready for review November 6, 2022 16:56
@sultan sultan marked this pull request as draft November 12, 2022 20:12
@sultan sultan marked this pull request as ready for review November 12, 2022 21:08
@sultan
Copy link
Contributor Author

sultan commented Nov 12, 2022

code:

documentation:

@sultan sultan mentioned this pull request Nov 12, 2022
Merged
@sultan
Copy link
Contributor Author

sultan commented Nov 12, 2022

@sultan sultan force-pushed the comparison branch 2 times, most recently from e467f85 to 9b33d69 Compare November 13, 2022 13:16
@sultan
Copy link
Contributor Author

sultan commented Nov 13, 2022

added another test case to prove website documentation right

@sultan
Copy link
Contributor Author

sultan commented Nov 13, 2022

code and documentation on par. waiting for final reviews.

elharo
elharo reviewed Nov 13, 2022
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still digging into the other PRs and the existing spec and code. Pleas ebear with me. It's been a while since I looked at this, and I need to make sure all the fiddly little bits are straight in my head.

maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java Outdated Show resolved Hide resolved
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java Outdated Show resolved Hide resolved
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java Outdated Show resolved Hide resolved
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java Outdated Show resolved Hide resolved
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java Outdated Show resolved Hide resolved
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java Outdated Show resolved Hide resolved
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java Outdated Show resolved Hide resolved
@sultan
Copy link
Contributor Author

sultan commented Nov 13, 2022

My major concern here is that ea and preview are new. Is it simply the case that their natural order already gives the correct results, both before and after this PR? If so, this is fine.

ea and preview removed and let treated with natural ordering

I also do not want to expand the public API in this PR. Looking at this code now I see at least one potential refactoring that would change that newly public API. In any case, new public API needs additional thought and discussion. It's a separate issue from fixing this bug.

removed the public visibilities

elharo
elharo previously requested changes Nov 14, 2022
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the output of the CI. The test failures look related.

@sultan
Copy link
Contributor Author

sultan commented Nov 14, 2022

made 3 changes.

  • javadoc without the last dot
  • put back a method that returns a string
  • fixed test, hopefully

@sultan
Copy link
Contributor Author

sultan commented Nov 29, 2022 •
edited
Loading

@elharo, rebased and applied code styles with spotless plugin

@sultan sultan requested a review from elharo November 29, 2022 12:13
@sultan sultan force-pushed the comparison branch 2 times, most recently from 7b078b3 to 8bd408c Compare November 29, 2022 12:39
@elharo elharo dismissed their stale review November 29, 2022 12:41

rereviewing

elharo
elharo reviewed Nov 29, 2022
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just some doc nits

maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java Outdated Show resolved Hide resolved
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java Show resolved Hide resolved
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java Outdated Show resolved Hide resolved
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java Outdated Show resolved Hide resolved
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java Outdated Show resolved Hide resolved
@sultan
Copy link
Contributor Author

sultan commented Nov 29, 2022

made the suggested change, thanks @elharo

@sultan
Copy link
Contributor Author

sultan commented Nov 29, 2022 •
edited
Loading

@elharo, fixed javadoc build errors by replacing > with &gt; and < with &lt;

@elharo
Copy link
Contributor

elharo commented Dec 3, 2022

All looks good. I need to copy the PR into my account to run it through Jenkins

@elharo elharo mentioned this pull request Dec 3, 2022
Closed
@elharo
Copy link
Contributor

elharo commented Dec 3, 2022

Jenkins passed my fork of this so I'm going to merge.

https://ci-maven.apache.org/job/Maven/job/maven-box/job/maven/job/MNG-7559/

@elharo elharo merged commit 97e8bf6 into apache:master Dec 3, 2022
@gnodet gnodet added this to the 4.0.0-alpha-3 milestone Dec 5, 2022
@sultan
Copy link
Contributor Author

sultan commented Dec 16, 2022

@elharo is it wished to port this to 3.9.x branch? if so a PR is available here:

@sultan sultan mentioned this pull request Dec 16, 2022
Closed
8 tasks
@elharo
Copy link
Contributor

elharo commented Dec 16, 2022

Check on the dev mailing list and see of anyone is planning another 3.9 release.

asfgit pushed a commit that referenced this pull request Dec 19, 2022
@michael-o
Revert "fix version comparison (#845)"
29e7b0e 
This reverts commit 97e8bf6.
@michael-o
Copy link
Member

This merge has been reverted on master.

@sultan
Copy link
Contributor Author

sultan commented Dec 20, 2022

This merge has been reverted on master.

A new clean PR was created to reintroduce the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-o michael-o michael-o left review comments

@elharo elharo elharo approved these changes

@slachiewicz slachiewicz slachiewicz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.0.0-alpha-3
Development

Successfully merging this pull request may close these issues.
5 participants
@sultan @elharo @michael-o @slachiewicz @gnodet