Add review bot tool for checkstyle

 






 

 


  Review Request #9488
  —
  Created Jan. 16, 2018 and submitted 

 


















 

  

   Information

  





 
 

  misery
 









 
 

  ReviewBot
 









 
 

  master
 









 
 

  
 









 
 

  
 









 
 

  b53ebe9...
 






 




 

  

   Reviewers

  





 
 

  reviewbot
 









 
 

  
 






 








 





  


 
 

  
Checkstyle is a development tool to help developers write

Java code that adheres to a coding standard. It automates

the process of checking Java code.

This makes it ideal for projects that want to enforce a

coding standard. 


Checkstyle can check many aspects of the source code.

It can find class design problems, method design problems.

It also has the ability to check code layout and formatting

issues. 


http://checkstyle.sourceforge.net/

 








  


 
 

  
Added checkstyle as tool and saw that it finds correct "errors".

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
Your description needs to be fleshed out a bit. Please see https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea for details.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Some of the older files may have this at the top, but we don't duplicate this in each file anymore. …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
I think you meant to write "Review Bot tool"

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
There may be many worker nodes, which may not all have the same config file present. Can we instead define …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
There's an extra > at the end of the line. This should also likely be "Checkstyle configuration file", though if …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
If there's some error when executing, will it give us valid XML to parse? Can this raise an exception that …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Can we call this checkstyle_jar_path (or even just checkstyle_path) for consistency with the PMD config value?

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
pass is unnecessary if there's another statement in the except handler.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Should this be "checkers for java code"?

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Please wrap to 80 columns.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Please wrap to 80 columns.

      		
      
       daviddavid
      
      
       
      
     


    
   

  

 







 






  

 



















 




 




 














 



 

  

   




misery




   

   

  

 



 















 





 

  

   




misery




   

   





  

 



 

  


   

   

    
 
misery


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 







  2. 
 
    
  
  
    
  
   
    
    
    




    
    checkstyle has a check for naming convention of java files. It checks if a class uses the same name for the filename. As the reviewbot create a temporary filename it will be an error for each run.
    

    "The name of the outer type and the file do not match."
    

    Is there an option to use a temporary directory with the real filename?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Ok, same problem with "package name". It needs a tempdir with the same source dir structure.
      
Package name is not same as directory.
      
 
      

      2. 


 
    


    

 
    

    3. 













  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




misery




   

   

  

 



 















 



 

  

   




misery




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 







  2. 
 
    
  
  
    
  
   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Your description needs to be fleshed out a bit. Please see https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea for details.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 















  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/checkstyle.py
      


       (Diff revision 4)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Some of the older files may have this at the top, but we don't duplicate this in each file anymore. If your company needs to claim copyright that can go in the COPYING file.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/checkstyle.py
      


       (Diff revision 4)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    I think you meant to write "Review Bot tool"
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/checkstyle.py
      


       (Diff revision 4)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    There may be many worker nodes, which may not all have the same config file present. Can we instead define the configuration XML in the options and write that out to a file while executing?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Yeah, ok... but a checkstyle config xml can be up to more than 5 or 10 kb! Is it possible to upload a file to the settings or use a textarea?
      

      By the way.... same problem is with pmd. Should be the same behaviour.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      You should be able to do a texarea by having:
      

      

      'field_options': {
    'widget': 'django.forms.Textarea',
    ...
},

      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Alternatively, I wonder if it'd be useful to have two options here:
      

        

      1. Provide raw XML text content to be written to disk.
        2. 

      2. Provide a URI to an XML file somewhere. If that's locally on disk, the URI will be file://. If it's remote, https://, etc.
        3. 

      

      Just an idea, but perhaps out of scope for now.
      
 
      

      4. 



    4. 
 
 
      
 
      
  
 
      
 
      
  
      I tried 'widget': 'django.forms.Textarea' but got no field anymore. Also I looked into JSHint checker and tried those. But it isn't working, too. Maybe JSHint is broken? I cannot see any textarea there.
      
 
      

      5. 



    5. 
 
 
      
 
      
  
 
      
 
      
  
      Ok, my mistake.... fixed. :-)
      

      @Mike
      
Yeah, nice idea. But that can be another changeset. :-)
      
 
      

      6. 


 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/checkstyle.py
      


       (Diff revision 4)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    There's an extra > at the end of the line. This should also likely be "Checkstyle configuration file", though if you make the change to have this be the config XML you will want to change the label/help text to match.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/checkstyle.py
      


       (Diff revision 4)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    If there's some error when executing, will it give us valid XML to parse? Can this raise an exception that should be caught?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 




  8. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 




  

 
















 



 

  

   




misery




   

   

  

 



 















 



 

  

   




misery




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/config.py
      


       (Diff revision 6)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Can we call this checkstyle_jar_path (or even just checkstyle_path) for consistency with the PMD config value?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/checkstyle.py
      


       (Diff revision 6)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    pass is unnecessary if there's another statement in the except handler.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Should this be "checkers for java code"?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Please wrap to 80 columns.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      uh, yeah, you should enable doc8 if it is merged. :-)
      
 
      

      2. 


 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Please wrap to 80 columns.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  7. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 




  

 
















 



 

  

   




misery




   

   

  

 



 















 



 

  

   




misery




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




misery




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to master (29282ba)





 


 










  

 









     

    

   

  

  

   
   Loading...