-
Notifications
You must be signed in to change notification settings - Fork 2.8k
abstract implicit used API MavenInvoker#customizeSettingsRequest
#2406
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
Conversation
MavenInvoker#customizeSettingsRequest
bb832d4 to
d0f4f31
Compare
MavenInvoker#customizeSettingsRequestMavenInvoker#customizeSettingsRequest
d0f4f31 to
83b1b18
Compare
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java
Show resolved
Hide resolved
MavenInvoker#customizeSettingsRequestabstract implicit (un)used API MavenInvoker#customizeSettingsRequest
abstract implicit (un)used API MavenInvoker#customizeSettingsRequestabstract implicit used API MavenInvoker#customizeSettingsRequest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any benefits in these changes, quite the opposite.
|
yes, the now explicitly methods are kind of overhead, as empty like before, would be nice to have them in some kind of default impl. Whats the matter? Its just the same method and signature, right? Empty methods will behave the same, assuming your concern is about architectural design? To me this change only shows the real intent of this method. Before it was a secret to override this methods, even tho its actually part of API, now as they are |
All protected methods can be overridden, that's no secret. It's a way to customize the request/result. As we see, it's usually not needed, so not sure why we'd have to force derived classes to provide two empty methods. |
agree. having empty methods and or unused params, still remains smell. |
depends on:
trycan useautomatic resource management1/2 #2401AbstractMojo#createFile(File)#2395Following this checklist to help us incorporate your
contribution quickly and easily:
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.
[MNG-XXX] SUMMARY,where you replace
MNG-XXXandSUMMARYwith the appropriate JIRA issue.[MNG-XXX] SUMMARY.Best practice is to use the JIRA issue title in both the pull request title and in the first line of the commit message.
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
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.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.