You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "elharo (via GitHub)" <gi...@apache.org> on 2025/01/10 13:07:35 UTC
[PR] Remove unused experimental version methods [maven]
elharo opened a new pull request, #2036:
URL: https://github.com/apache/maven/pull/2036
If it turns out we do need these, they should be renamed getLowestVersion and getHighestVersion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8485] Remove unused, untested, experimental version methods [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #2036:
URL: https://github.com/apache/maven/pull/2036#issuecomment-2582741573
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 they do is return the first and last elements of a list. That can easily be done with the usual methods of java.util.List. They are at best very minor conveniences.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8485] Remove unused, untested, experimental version methods [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #2036:
URL: https://github.com/apache/maven/pull/2036#issuecomment-2582684770
-1 This is API and this is a requirement (and required functionality). This is plugin facing, no matter it is unused in maven itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8485] Remove unused, untested, experimental version methods [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #2036:
URL: https://github.com/apache/maven/pull/2036#issuecomment-2583341336
> 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:
https://github.com/apache/maven/blob/13ad6376e00aa5eb28f41a84e0dd06bb1f045c2e/impl/maven-impl/src/main/java/org/apache/maven/internal/impl/resolver/DefaultModelResolver.java#L138-L150
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8485] Remove unused, untested, experimental version methods [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #2036:
URL: https://github.com/apache/maven/pull/2036#issuecomment-2582688251
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8485] Remove unused, untested, experimental version methods [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #2036:
URL: https://github.com/apache/maven/pull/2036#issuecomment-2582701020
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8485] Remove unused, untested, experimental version methods [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #2036:
URL: https://github.com/apache/maven/pull/2036#issuecomment-2582693926
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8485] Remove unused, untested, experimental version methods [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #2036:
URL: https://github.com/apache/maven/pull/2036#issuecomment-2582697015
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8485] Remove unused, untested, experimental version methods [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #2036:
URL: https://github.com/apache/maven/pull/2036#issuecomment-2583198372
> 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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8485] Remove unused, untested, experimental version methods [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #2036:
URL: https://github.com/apache/maven/pull/2036#issuecomment-2583313108
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8485] Remove unused, untested, experimental version methods [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #2036:
URL: https://github.com/apache/maven/pull/2036#issuecomment-2586354068
Superseded by #2039
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8485] Remove unused, untested, experimental version methods [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet closed pull request #2036: [MNG-8485] Remove unused, untested, experimental version methods
URL: https://github.com/apache/maven/pull/2036
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8485] Remove unused, untested, experimental version methods [maven]
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #2036:
URL: https://github.com/apache/maven/pull/2036#issuecomment-2952855578
source branch deleted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org