Add a shellcheck tool for Review Bot

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

ceciliawei
ReviewBot
master
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
  1. 
      
  2. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     

    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. bot/setup.py (Diff revision 1)
     
     

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

  4. 
      
ceciliawei
  1. 
      
  2. 
      
ceciliawei
ceciliawei
david
  1. This is looking excellent!

  2. bot/reviewbot/tools/shellcheck.py (Diff revision 3)
     
     

    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:

      1
      2
      3
      4
      #!/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. 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. 
      
ceciliawei
david
  1. 
      
  2. bot/reviewbot/tools/shellcheck.py (Diff revision 4)
     
     

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

  3. bot/reviewbot/tools/shellcheck.py (Diff revision 4)
     
     

    Please add a period to the end of this comment.

  4. bot/reviewbot/tools/shellcheck.py (Diff revision 4)
     
     
     

    A couple comments here:

    1. Since we're using regex.search, we shouldn't need to strip the end of the line.
    2. Please add a blank line between these two (so there's some space above the if block)
    3. We can just use self.shebang_regex instead of first assigning it to a local variable.
  5. 
      
ceciliawei
keanweng
  1. 
      
  2. bot/reviewbot/tools/shellcheck.py (Diff revision 5)
     
     

    There is an extra line here.

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

  3. 
      
david
  1. Ship It!
  2. 
      
jblazusi
  1. Ran it manually and it worked great, just one minor issue when used against python 2.7

  2. bot/reviewbot/tools/shellcheck.py (Diff revision 5)
     
     

    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.

  3. 
      
ceciliawei
ceciliawei
  1. 
      
  2. 
      
ceciliawei
  1. 
      
  2. 
      
jblazusi
  1. Ship It!
  2. 
      
jblazusi
  1. 
      
  2. bot/reviewbot/tools/shellcheck.py (Diff revision 6)
     
     

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

  3. 
      
ceciliawei
david
  1. Ship It!
  2. 
      
ceciliawei
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (17b2b46)
Loading...