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

Refactoring: coalesce use-*-* goals. #1202

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrzejj0
Copy link
Contributor

@andrzejj0 andrzejj0 commented Dec 24, 2024

I wanted to try and do something similar to what #292 was, but since the plugin has moved on, it first needs some refactoring.

This PR makes the use-*-* goals use shared codebase in order to make future refactoring simpler.

@andrzejj0 andrzejj0 self-assigned this Dec 24, 2024
@andrzejj0 andrzejj0 marked this pull request as draft December 24, 2024 11:18
@andrzejj0 andrzejj0 marked this pull request as ready for review December 24, 2024 11:25

@Override
protected boolean isAllowSnapshots() {
return true;
Copy link
Member

Choose a reason for hiding this comment

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

why such ...

Copy link
Contributor Author

@andrzejj0 andrzejj0 Dec 24, 2024

Choose a reason for hiding this comment

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

This is called by a method from the base class, UseLatestVersionsMojoBase#update so that it selects snapshots -- we are looking for snapshots to upgrade to:

                Optional<ArtifactVersion> newestVer = versionProducer(Arrays.stream(versions.getNewerVersions(
                                dep.getVersion(), unchangedSegment, isAllowSnapshots(), isAllowDowngrade()))
                        .filter(this::artifactVersionsFilter));

So, to make this execution universal and common to different sorts of "use-*-*", the method in the base class uses those callback methods.

As an alternative, this could be done using composition and an object having those parameters (allowSnaphots) as its constructor parameters.

Copy link
Contributor Author

@andrzejj0 andrzejj0 Dec 24, 2024

Choose a reason for hiding this comment

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

The property allowSnapshots doesn't make sense in some of the goals, like this one (because here we are only interested in snapshots, so we must allow them), so I'll push the property down the class hierarchy to goals where it does make sense. At the same time, I'll pull isAllowSnapshots() upwards as an abstract method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note that I'm doing this in stages so that the PR doesn't grow too big.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants