-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
[MNG-5222] - Maven 3 no longer logs warnings about deprecated parameter in plugin #285
Conversation
Do you have any example of a plugin where you can check the effect of this change? |
m-compiler-p has some deprecated parameters. |
ok, looks good to me.
|
if ( parameter.getExpression() != null ) | ||
{ | ||
String userParam = parameter.getExpression().replace( "${", "'" ).replace( '}', '\'' ); | ||
sb.append( " (User Parameter " ).append( userParam ).append( ")" ); |
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.
It is called User Property, not User Parameter.
Also be aware that the difference between expression and default-value wasn't that clear in the past, so there are still plugins around with odd values. So only show it in case of a valid expression.
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.
Do you have and example of such an old plugin? How do I know if it is a "valid expression" at this stage of the plugin execution?
maven-core/src/main/java/org/apache/maven/plugin/internal/ValidatingConfigurationListener.java
Outdated
Show resolved
Hide resolved
while working on MNG-5001 (failure on read-only parameters), I imagine I'll work on this one also question: do you have an example of deprecated parameter on a plugin available in the wild (ideally Maven 2 compatible) so we can easily test behaviour with misc Maven versions, please? |
@hboutemy please see maven-compiler-plugin. It has deprecated |
Is there still interest in this for Maven 4? |
I hope it is interested also for 3.x |
1 similar comment
I hope it is interested also for 3.x |
@belingueres Thanks for your contributions. First I kindly ask you to rebase with current code base - we will see if build pass. |
c9f8dd7
to
cd3beee
Compare
@slawekjaranowski Rebased and changed logger for SLF4J. |
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.
Responsibility of ValidatingConfigurationListener
is to check configurations - so I don't mix it ...
Proposition
create class: CompositeConfigurationListener
implements ConfigurationListener
- with
constructor (List<ConfigurationListener> listeners )
- in implementation we call each listener
We can leave DebugConfigurationListener
for show debug values
and create next DeprecatedConfigurationListener
with logging about deprecated params
maven-core/src/main/java/org/apache/maven/plugin/internal/ValidatingConfigurationListener.java
Outdated
Show resolved
Hide resolved
maven-core/src/main/java/org/apache/maven/plugin/internal/ValidatingConfigurationListener.java
Outdated
Show resolved
Hide resolved
maven-core/src/main/java/org/apache/maven/plugin/internal/ValidatingConfigurationListener.java
Outdated
Show resolved
Hide resolved
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 consulted with other and reviewed again, there are not many lines of code here, so in this simple case is acceptable to put in one class.
Class DebugConfigurationListener can be removed.
Pleas look at rest of my comments.
maven-core/src/main/java/org/apache/maven/plugin/internal/ValidatingConfigurationListener.java
Outdated
Show resolved
Hide resolved
maven-core/src/main/java/org/apache/maven/plugin/internal/ValidatingConfigurationListener.java
Outdated
Show resolved
Hide resolved
maven-core/src/main/java/org/apache/maven/plugin/internal/ValidatingConfigurationListener.java
Outdated
Show resolved
Hide resolved
Summarize current discussion:
and will be ok for me |
@belingueres thanks - please squash to one commit it will be easier for cherry pick |
Feel free to drop my branch - that was just quick rebase of original pr. |
parameters - Added warning when setting deprecated parameter with value different than the default. - Changed Logger to SLF4J.
Branch for change created with simple fix after IT tests - #705 |
@belingueres Thanks for contributing - your commit and credential will be preserved in my PR, this one can be closed. |
parameters
than the default.
Following 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] - Fixes bug in ApproximateQuantiles
,where you replace
MNG-XXX
with the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verify
to 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.