- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.7k
Fix(#2769): Modify the annotation processor in 2.x to give a warning if a plugin builder attribute does not have a public setter. #3195
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
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.
Thanks for your contribution! I added some minor remarks below.
If you haven't done it already, please sign the ASF ICLA.
        
          
                .../org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                .../org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                .../org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                .../org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...re/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...re/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | Sounds good @ppkarwasz let me fix and rebase this! 🙂 | 
50b1e54    to
    d6e4898      
    Compare
  
    d6e4898    to
    024654e      
    Compare
  
    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.
LGTM, thanks!
        
          
                ...re/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | I sent you an invitation to the Apache Slack to the address you use in your commits. | 
| Hi @ppkarwasz the build was failing, hence I put the pr in draft 🥲. Thanks for approving it, but I will have to make a revision. I am not sure I got the slack invitation, was the email [email protected]? | 
| No, I used the one from you University/Alma Mater that you use for your commits. | 
| Hi @ppkarwasz i am trying to move away from that email, because I don't have access to that anymore 🥲 will change it in my commits as well! I apologize | 
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.
Hi @jaykataria1111,
The OSGi fail is due to an "unauthorized" usage of Commons Lang 3. All optional Maven dependencies, must be explicitly marked as optional in OSGi, if we need them. In this case we don't.
        
          
                ...re/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...re/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...re/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      a4dc84d    to
    0cc7f96      
    Compare
  
    …rning if a plugin builder attribute does not have a public setter.
0cc7f96    to
    1203725      
    Compare
  
    | Hi @ppkarwasz addressed all the comments here we should be good :) | 
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.
Thank you! I'll merge the PR, when the CI checks succeed.
The following commit modifies the annotation processor in 2.x to shell out a warning if the plugin builder attribute does not have a public setter.
A
@SuppressWarnings("log4j.public.setter")attribute can be used to ignore this compilation warning incase it is a known issue.Checklist
2.xbranch if you are targeting Log4j 2; usemainotherwise./mvnw verifysucceeds (if it fails due to code formatting issues reported by Spotless, simply run./mvnw spotless:applyand retry)src/changelog/.2.x.xdirectory