Add a shellcheck tool for Review Bot
Review Request #11246 — Created Oct. 26, 2020 and submitted
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 |
---|---|---|
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__) … |
![]() |
|
I believe there should be a trailing ',' here. |
![]() |
|
I feel like a lot of shell scripts may not have an extension at all. Maybe instead we should look … |
|
|
Let's say "Using a POSIX-sh compatible shell" |
|
|
Please add a period to the end of this comment. |
|
|
A couple comments here: Since we're using regex.search, we shouldn't need to strip the end of the line. Please add … |
|
|
There is an extra line here. |
|
|
I noticed that this call to super() is not compatible with python 2.7, because it expects parameters. I believe that … |
![]() |
|
Similarily to other reviews, this execute function should be placed inside of a try/except. |
![]() |

-
-
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.
-
Change Summary:
Addressed feedback
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+248) |
Checks run (2 succeeded)
Change Summary:
Updated default severity level.
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+248) |
Checks run (2 succeeded)
-
This is looking excellent!
-
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?
Change Summary:
Detect shell scripts using shebang
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+302) |
Checks run (2 succeeded)
-
-
-
-
bot/reviewbot/tools/shellcheck.py (Diff revision 4) A couple comments here:
- Since we're using regex.search, we shouldn't need to strip the end of the line.
- Please add a blank line between these two (so there's some space above the if block)
- We can just use
self.shebang_regex
instead of first assigning it to a local variable.
Change Summary:
Addressed comments.
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+300) |
Checks run (2 succeeded)

-
Ran it manually and it worked great, just one minor issue when used against python 2.7
-
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 besuper(ShellCheckTool, self).__init__()
. I saw that a similar approach was being done in the JSHint tool.
Change Summary:
Make the super() method python2.7 compatible.
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+300) |
Checks run (2 succeeded)

-
-
bot/reviewbot/tools/shellcheck.py (Diff revision 6) Similarily to other reviews, this
execute
function should be placed inside of a try/except.
Change Summary:
Addressed feedback
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+304) |