• 
  
    
 
    
 
    

    



    
    
    
    
     
    




    
 
    

  
    



    
 

 

  

  

 

    


  
    





    
 
    



  

    
 
 
    
  
    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. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       docs/reviewbot/reviewbot/tools/checkstyle.rst
       


        (Diff revision 6)


       
      
     
      
          
    
     
       
      

     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      Should this be "checkers for java code"?
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      5. 






    5. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       docs/reviewbot/reviewbot/tools/checkstyle.rst
       


        (Diff revision 6)


       
      
     
      
          
    
     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      Please wrap to 80 columns.
      
  
      
 
      

 
      
  


      
 
      
 
        

  


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

        2. 


 
      


      

 
      

      6. 






    6. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       docs/reviewbot/reviewbot/tools/checkstyle.rst
       


        (Diff revision 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:
   
    
   
    

    Completed

   
    
  
    



  
    
   
    
    Change Summary:
   
    
   
    
    
    Pushed to master (29282ba)