Add a shellcheck tool for Review Bot

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

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
Loading file attachments...

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
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
Review request changed

Change Summary:

Addressed comments.

Commits:

Summary Author
-
Add a shellcheck tool for Review Bot.
ceciliaccwei
+
Add a shellcheck tool for Review Bot.
ceciliaccwei

Diff:

Revision 5 (+300)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
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. 
      
Loading...