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.

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

#632 Fixed lower and upper bounds to keep milestones and rcs in the right majors. #672

Merged
merged 1 commit into from Sep 19, 2022

Conversation

sultan
Copy link
Contributor

@sultan sultan commented Sep 7, 2022

#632 Fix Bounds so lower and upper bounds keep milestones and rcs in the same majors when they are.

a quick before/after :

image
image

@sultan sultan changed the title #632 Fix Bounds so lower and upper bounds keep milestones and rcs in the same majors when they are. #632 Fixed lower and upper bounds to keep milestones and rcs in the same majors. Sep 14, 2022
@sultan sultan changed the title #632 Fixed lower and upper bounds to keep milestones and rcs in the same majors. #632 Fixed lower and upper bounds to keep milestones and rcs in the right majors. Sep 14, 2022
@sultan sultan marked this pull request as draft September 16, 2022 16:00
@sultan sultan force-pushed the PreviewFlag branch 3 times, most recently from ada7f3b to 9a0a4de Compare September 16, 2022 16:40
@sultan sultan marked this pull request as ready for review September 16, 2022 17:07
@sultan
Copy link
Contributor Author

sultan commented Sep 16, 2022

basically this PR adds a "forceSnapshot" boolean to the increaseSegment method. this allows to create bounds that includes 4.0-snaphost 4.0-rc 4.0-m1 4.0-final in the same 4.0 major, for that we need that boolean to increase 3.0 into lower bound 4.0-snaphost included, and upper bound 5.0-snapshot excluded.

another PR is on the way to basically ease the exclusions of such "previews" builds.

@sultan sultan marked this pull request as draft September 16, 2022 18:32
@jarmoniuk
Copy link
Contributor

jarmoniuk commented Sep 17, 2022

Hi @sultan do you know if you still have much to do there? I'm working on a refactor which includes changes to UpdateScope (which may lead all the way to getting rid of it). There's a lot of old cruft which needs to be purged. Or maybe we could sync?

Here's the diff:

#702

@sultan
Copy link
Contributor Author

sultan commented Sep 17, 2022

All good. No more job needed on my side for this PR.
I was hoping that these rc ordering things could be implemented in Maven git repo (their artifacts comparator)
If I ever succeed in persuading them to include rc ordering, maybe this job would have to be rethought.

Agreeing on removing unused method, or a cleanup/refactoring.

@sultan sultan marked this pull request as ready for review September 17, 2022 09:10
@sultan
Copy link
Contributor Author

sultan commented Sep 17, 2022

I can’t have access to my computer this morning. If you can merge both today I’m fine. Though I can help a bit later

@jarmoniuk
Copy link
Contributor

Sure. If you still want to sync, let me know, we can merge our branches.

@sultan
Copy link
Contributor Author

sultan commented Sep 17, 2022

I'll try something in about 6 hours if it can help

@jarmoniuk
Copy link
Contributor

jarmoniuk commented Sep 17, 2022

If you go and check in my branch, UpdateScope is no more 😆

@sultan
Copy link
Contributor Author

sultan commented Sep 17, 2022

great job of factorization. i'll see if i can merge all of it in my PR tomorrow

@sultan sultan force-pushed the PreviewFlag branch 2 times, most recently from c2a23ed to 2fc0d46 Compare September 18, 2022 12:20
@sultan
Copy link
Contributor Author

sultan commented Sep 18, 2022

rebased and ready.

@sultan
Copy link
Contributor Author

sultan commented Sep 18, 2022

if you need to merge #708 first, then there is room for me to improve my PR, it seems the forceSnaphsot will be always true after all. then i can simplify everything. and maybe rebase my PR onto master after 707 and 708 are merged ?

@slawekjaranowski
Copy link
Member

@sultan #708 merged

@sultan
Copy link
Contributor Author

sultan commented Sep 18, 2022

Thanks !

can Mercury versions allow snapshot qualifier ?
do i need to strip snapshot qualifier before incrementing a NumericVersion ?

is it planned to drop support for Mercury ?

@sultan
Copy link
Contributor Author

sultan commented Sep 18, 2022

rebased but requires your attention on above questions before merging

@jarmoniuk
Copy link
Contributor

jarmoniuk commented Sep 18, 2022

Looks like it did support -SNAPSHOT and similar qualifiers.

https://www.mail-archive.com/dev@maven.apache.org/msg77826.html

https://www.mail-archive.com/dev@maven.apache.org/msg76344.html

And there's also https://github.com/apache/maven-mercury

Maybe it's best to ask @olamy ?

Version is specified in https://docs.osgi.org/download/r4v41/r4.core.pdf, §3.2.4 "Version"

@sultan
Copy link
Contributor Author

sultan commented Sep 18, 2022

Thanks ! the PR currently generates bounds with a "-SNAPSHOT" qualifier, always, regardless of comparator (Maven, Mercury, NumericVersion). the current result is tested against all three Test classes which I modified to expect always the snapshot qualifier. if these Tests classes are okay for the reviewers, then the PR can be merged.
my tests on Maven poms are okay.

image

@slawekjaranowski
Copy link
Member

In the latest picture we have:

4.0.0-RC1 Next Major
4.0.0-RC2

Should 4.0.0-RC2 will be Next Major instead of 4.0.0-RC1?

@sultan
Copy link
Contributor Author

sultan commented Sep 18, 2022

Not sure about differentiating RC1 vs RC2. the question of differentiating RC1 vs M1 however is another issue that may be adressed in another PR. the problem is caused by dashes and dots not treated the same way.

The result could better be M1 Next Major followed by RC1 RC2.

this problem was already there before the PR. I know the involved class but I did not investigate the solution.

The improvement done in this PR is having M1 RC1 RC2 and Realease labelled in the same major.

I'm open to improvement if you can provide a textual expected rendering.

@sultan
Copy link
Contributor Author

sultan commented Sep 19, 2022

thanks @slawekjaranowski for the merge with the master branch.
i did it on my own branch to avoid losing your push next time.

as a result of fixing #632, it appears we could maybe get rid of the class MajorMinorIncrementalFilter which states that its very existence is filtering milestones and RC from the result of the calls, when they are in the wrong major, and are now already filtered out by my PR.
i am investigating this in another draft.

meanwhile this PR is still ready for review/merge.

Thank you for your time.

@slawekjaranowski slawekjaranowski added this to the 2.13.0 milestone Sep 19, 2022
@slawekjaranowski slawekjaranowski merged commit f0dc715 into mojohaus:master Sep 19, 2022
sultan added a commit to sultan/versions-maven-plugin that referenced this pull request Sep 28, 2022
sultan added a commit to sultan/versions-maven-plugin that referenced this pull request Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting valid versions comparison in case of qualifiers (alpha, beta, milestone, rc)
3 participants