[WIP]ReviewBot ShellCheckTool

Review Request #7197 — Created April 12, 2015 and discarded

Information

ReviewBot
release-0.2.x

Reviewers

ReviewBot ShellCheckTool

apt-get install cabal-install
cabal update
cabal install shellcheck
ln -s ~/.cabal/bin/shellcheck ~/usr/local/bin/shellcheck

Description From Last Updated

We don't use module-level docstrings.

brenniebrennie

We organize our imports into three groups Python standard library imports Third party imports Project imports Given that, can we …

brenniebrennie

"tool" not "model".

brenniebrennie

Single quotes around sh.

brenniebrennie

This sentence is implied by the first.

brenniebrennie

A few grammar mistakes here, how about: """Use ShellCheck to analyze the file and post the result."""

brenniebrennie

Blank line between end of block and statement.

brenniebrennie

Blank line between statement and block.

brenniebrennie

Blank line between these.

brenniebrennie

Can we pull this out into a variable like shellcheck_cmd and then we can format the execute like: output = …

brenniebrennie

You probably want none_on_ignored_error=True as well.

brenniebrennie

Blank line between statement and block.

brenniebrennie

What is cmm? This needs a better name.

brenniebrennie

Blank line between statement and block.

brenniebrennie

Can you include some sample output of shellcheck?

brenniebrennie

Blank line between statement and block. Also, can you make this regular expression a compiled constant on the class using …

brenniebrennie

Can you also compile this? Please document what it does also.

brenniebrennie

Blank line between two blocks. If we are expecting a lot of strings to join, we should make cmm an …

brenniebrennie

This should not be included in your change.

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        bot/reviewbot/tools/shellcheck.py
        bot/setup.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        bot/reviewbot/tools/shellcheck.py
        bot/setup.py
    
    
  2. 
      
brennie
  1. 
      
  2. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
     
    Show all issues

    We don't use module-level docstrings.

  3. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    We organize our imports into three groups

    1. Python standard library imports
    2. Third party imports
    3. Project imports

    Given that, can we reformat these imports as

    import re
    
    from reviewbot.tools import Tool
    # ...
    
  4. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
    Show all issues

    "tool" not "model".

  5. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
    Show all issues

    Single quotes around sh.

  6. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
    Show all issues

    This sentence is implied by the first.

  7. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    A few grammar mistakes here, how about:

    """Use ShellCheck to analyze the file and post the result."""

  8. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
     
    Show all issues

    Blank line between end of block and statement.

  9. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
     
    Show all issues

    Blank line between statement and block.

  10. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
     
    Show all issues

    Blank line between these.

  11. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues

    Can we pull this out into a variable like shellcheck_cmd and then we can format the execute like:

    output = execute(shellcheck_cmd, split_lines=True, ignore_errors=True)
    
  12. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
    Show all issues

    You probably want none_on_ignored_error=True as well.

  13. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
     
    Show all issues

    Blank line between statement and block.

  14. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
    Show all issues

    What is cmm? This needs a better name.

  15. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
     
    Show all issues

    Blank line between statement and block.

  16. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
    Show all issues

    Can you include some sample output of shellcheck?

  17. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
     
    Show all issues

    Blank line between statement and block.

    Also, can you make this regular expression a compiled constant on the class using re.compile? Please document what it does also.

  18. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
    Show all issues

    Can you also compile this? Please document what it does also.

  19. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
     
    Show all issues

    Blank line between two blocks.

    If we are expecting a lot of strings to join, we should make cmm an array and then use ''.join(cmm) when we go to comment.

  20. bot/setup.py (Diff revision 1)
     
     
    Show all issues

    This should not be included in your change.

  21. 
      
CA
Review request changed

Status: Discarded

Change Summary:

Discarded in favour of https://reviews.reviewboard.org/r/7701.

Loading...