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 Author 
Add a shellcheck tool for Review Bot.
 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...