[MNG-8485] Remove unused, untested, experimental version methods

[elharo]

YAGNI.
If it turns out we do need these, they should be renamed getLowestVersion and getHighestVersion.

[cstamas]

-1 This is API and this is a requirement (and required functionality). This is plugin facing, no matter it is unused in maven itself.

[elharo]

Then why is it experimental? Fish or cut bait. Either remove the Experimental annotation and add tests, or take it out.

Also, as noted above, the methods are confusingly named.

[elharo]

Also needs API doc. And now that I look more closely, it might be buggy. I don't see anything that guarantees the zeroth version in the list is the lowest version.

[cstamas]

Re naming: good, rename them.
But I think we already explained it: whole Maven API is marked as "experimental" on purpose. And yes, you are gonna need it as in Maven4 plugins you cannot access resolver or maven-artifact, ONLY this API.

[gnodet]

Also needs API doc. And now that I look more closely, it might be buggy. I don't see anything that guarantees the zeroth version in the list is the lowest version.

That should be in the javadoc for getVersions. The list returned by the resolver is always ordered.

[elharo]

The list is ordered, yes. All lists are ordered. But is the list ordered reproducibly by version? I see nothing that guarantees that.

I'm also fairly confident that plugins don't actually need these two methods. All these methods do is return the first and last elements of a list and wrap it in an Optional. That can easily be done with the usual methods of java.util.List. They are at best very minor conveniences.

[gnodet]

The list is ordered, yes. All lists are ordered. But is the list ordered reproducibly by version? I see nothing that guarantees that.

Sorry, I meant the list is sorted.
https://github.com/apache/maven-resolver/blob/1c580b0165e73492e7cf23a88f548056e07e2559/maven-resolver-api/src/main/java/org/eclipse/aether/resolution/VersionRangeResult.java#L97-L104
This class is currently a wrapper on top of the resolver class.

I'm also fairly confident that plugins don't actually need these two methods. All these methods do is return the first and last elements of a list and wrap it in an Optional. That can easily be done with the usual methods of java.util.List. They are at best very minor conveniences.

Right, that's clearly a helper method. Are those forbidden by YAGNI ? The principles you keep mentioning are usually fine when you have a closed piece of software, but it seems they do not apply very well on libraries.

And of course the code can be inlined, but you can't simplify it more than it is:

        return getVersions().isEmpty()
                ? Optional.empty()
                : Optional.of(getVersions().get(getVersions().size() - 1));

we don't have the new SequencedCollection from JDK 21.

How are those methods different from Objects.requireNonNull() for example ? The JDK API is full of helper methods... Are you saying they're worthless ?

[elharo]

You're putting words in my mouth. I'm saying these two methods are not needed, and they certainly aren't needed here.

As to libraries, yes, YAGNI applies to libraries, as I've been saying for a rather long time:

https://www.artima.com/articles/design-principles-and-xom

Swiss Army knife is a common antipattern for library design. Keep libraries focused. BaseRequest and Maven should not be in the business of selecting the first and last elements from a list. List.getFirst and List.getLast already exist in JDK 21. That's all anyone needs. Adding more methods in different projects and packages to do exactly the same thing just makes code harder to follow.

[gnodet]

You're putting words in my mouth. I'm saying these two methods are not needed, and they certainly aren't needed here.

As to libraries, yes, YAGNI applies to libraries, as I've been saying for a rather long time:

https://www.artima.com/articles/design-principles-and-xom

Swiss Army knife is a common antipattern for library design. Keep libraries focused. BaseRequest and Maven should not be in the business of selecting the first and last elements from a list. List.getFirst and List.getLast already exist in JDK 21. That's all anyone needs. Adding more methods in different projects and packages to do exactly the same thing just makes code harder to follow.

I disagree. Usually, what you want out of a version range resolution is the highest version. You usually don't care about the list of all possible versions.
It's weird that the most important data conveyed by the class would be hidden.

If that looks better, I'm fine with removing the default code and move them down to the implementation. But the fact that they can be computed is imho irrelevant. A lot of things is about computing some data out of existing data.

Fwiw, it should actually be used at this place:

maven/impl/maven-impl/src/main/java/org/apache/maven/internal/impl/resolver/DefaultModelResolver.java

Lines 138 to 150 in 13ad637

	List<Version> versions = session.resolveVersionRange(coords, repositories);
	if (versions.isEmpty()) {
	throw new ModelResolverException(
	"No versions matched the requested " + (type != null ? type + " " : "") + "version range '"
	+ version + "'",
	groupId,
	artifactId,
	version);
	}
	String newVersion = versions.get(versions.size() - 1).asString();
	if (!version.equals(newVersion)) {
	resolvedVersion.accept(newVersion);
	}

Not sure if this goes into my or your direction. Obviously, it's not required, but I think it would make the code more clear and readable, because that's usually the real piece of data we're interested in.

The above code needs to be refactored and either use a VersionRangeResolverResult or add a method on the session to resolve and return the highest version.

[gnodet]

Superseded by #2039

[slawekjaranowski]

source branch deleted

[jira-importer]

Resolve #9951