Add a shellcheck tool for Review Bot

 






 

 


  Review Request #11246
  —
  Created Oct. 26, 2020 and submitted 

 




















 

  

   Information

  





 
 

  ceciliawei
 









 
 

  ReviewBot
 









 
 

  master
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewbot, students
 









 
 

  jace, jblazusi
 






 








 





  


 
 

  
This adds support for shellcheck through Review Bot. ShellCheck is a

shell script static analysis tool that gives warnings and suggestions

for bash/sh shell scripts.

 








  


 
 

  
Configured the tool and ran it manually.

 







 









 





  

 


 
 

  



 

  
   


    Summary
    ID

    Author

   

  
  

   


    

     

      Add a shellcheck tool for Review Bot.
      
This adds support for shellcheck through Review Bot. ShellCheck is a
shell script static analysis tool that gives warnings and suggestions
for bash/sh shell scripts.

Testing done:
Configured the tool and ran it manually.

Reviewed at https://reviews.reviewboard.org/r/11246/

     


    		
    e093d80e68c8beada38b5d29dd7a8190025521ce

    ceciliaccwei

   



  
 




 













 

  
  


   

    

     

     

     

    

   


   

  

 







 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
I noticed that in files where I saw import logging, I also noticed that this was included: logger = logging.getLogger(__name__) …

      		
      
       jblazusijblazusi
      
      
       
      
     




     

      
       
       
I believe there should be a trailing ',' here.

      		
      
       jblazusijblazusi
      
      
       
      
     




     

      
       
       
I feel like a lot of shell scripts may not have an extension at all. Maybe instead we should look …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Let's say "Using a POSIX-sh compatible shell"

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Please add a period to the end of this comment.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
A couple comments here: Since we're using regex.search, we shouldn't need to strip the end of the line. Please add …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
There is an extra line here.

      		
      
       keanwengkeanweng
      
      
       
      
     




     

      
       
       
I noticed that this call to super() is not compatible with python 2.7, because it expects parameters. I believe that …

      		
      
       jblazusijblazusi
      
      
       
      
     




     

      
       
       
Similarily to other reviews, this execute function should be placed inside of a try/except.

      		
      
       jblazusijblazusi
      
      
       
      
     


    
   

  

 







 






  

 



















 




 




 














 





 

  

   




jblazusi




   

   







  

 



 

  


   

   

    
 
jblazusi


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/shellcheck.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    I noticed that in files where I saw import logging, I also noticed that this was included:
    

    logger = logging.getLogger(__name__)
    

    In order to name the logger, so that it is easier to track errors, later on.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/setup.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    I believe there should be a trailing ',' here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 





 

  

   




ceciliawei




   

   





  

 



 

  


   

   

    
 
ceciliawei


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




ceciliawei




   

   

  

 



 















 



 

  

   




ceciliawei




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    This is looking excellent!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/shellcheck.py
      


       (Diff revision 3)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    I feel like a lot of shell scripts may not have an extension at all. Maybe instead we should look at the first line to see if there's a shebang with a supported shell?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I did not realize this. Thanks for pointing out!
      
I searched up on the shebang, looks like the ones below are the most common used in shell scripts:
      

      #!/bin/sh – Execute the file using the Bourne shell, or a compatible shell, assumed to be in the /bin directory
#!/bin/bash – Execute the file using the Bash shell
#!/usr/bin/env bash - Using the env utility
#!/usr/bin/env sh - Using the env utility

      

      Currently I'm thinking of using these as the patterns to match the shebang. Another thing is that ShellCheck supports dash and ksh in addition to sh and bash. But I am not sure how commonly are those two being used in industries. Do you have any insight on if we should support those two as well?
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      In my experience, dash is sometimes used by people for their interactive terminals but I've never actually seen a script written in it. ksh used to be quite common, especially on BSD-based systems, but it's been a long time since it was popular and I suspect there aren't a lot of people writing scripts in it these days. Let's just do bash/sh for now, and it'll be easy enough to add dash and ksh later if people ask for them.
      
 
      

      3. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




ceciliawei




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/shellcheck.py
      


       (Diff revision 4)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Let's say "Using a POSIX-sh compatible shell"
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/shellcheck.py
      


       (Diff revision 4)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Please add a period to the end of this comment.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/shellcheck.py
      


       (Diff revision 4)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    A couple comments here:
    

      

    1. Since we're using regex.search, we shouldn't need to strip the end of the line.
      2. 

    2. Please add a blank line between these two (so there's some space above the if block)
      3. 

    3. We can just use self.shebang_regex instead of first assigning it to a local variable.
      4. 

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 



 

  

   




ceciliawei




   

   

  

 



 















 





 

  

   




keanweng




   

   







  

 



 

  


   

   

    
 
keanweng


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/shellcheck.py
      


       (Diff revision 5)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    There is an extra line here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Thanks for the review! This is actually the style used in all the ReviewBot tools.
      
 
      

      2. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




jblazusi




   

   







  

 



 

  


   

   

    
 
jblazusi


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ran it manually and it worked great, just one minor issue when used against python 2.7
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/shellcheck.py
      


       (Diff revision 5)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    I noticed that this call to super() is not compatible with python 2.7, because it expects parameters. I believe that in order for this to be compatible with python 2.7 it should be super(ShellCheckTool, self).__init__(). I saw that a similar approach was being done in the JSHint tool.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Thanks for catching that. I didn't realize if I do setup develop for python2.7 first and then 3.x versions, the tools will be ran in the last version that got setup. The syntax has now been updated to fit both python2/3.
      
 
      

      2. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




ceciliawei




   

   

  

 



 















 





 

  

   




ceciliawei




   

   





  

 



 

  


   

   

    
 
ceciliawei


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




ceciliawei




   

   





  

 



 

  


   

   

    
 
ceciliawei


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




jblazusi




   

   







  

 



 

  


   

   

    
 
jblazusi


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




jblazusi




   

   







  

 



 

  


   

   

    
 
jblazusi


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      bot/reviewbot/tools/shellcheck.py
      


       (Diff revision 6)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Similarily to other reviews, this execute function should be placed inside of a try/except.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




ceciliawei




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




ceciliawei




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-2.0.x (17b2b46)





 


 










  

 









     

    

   

  

  

   
   Loading...