• 
      

    [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.

    brennie brennie

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

    brennie brennie

    "tool" not "model".

    brennie brennie

    Single quotes around sh.

    brennie brennie

    This sentence is implied by the first.

    brennie brennie

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

    brennie brennie

    Blank line between end of block and statement.

    brennie brennie

    Blank line between statement and block.

    brennie brennie

    Blank line between these.

    brennie brennie

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

    brennie brennie

    You probably want none_on_ignored_error=True as well.

    brennie brennie

    Blank line between statement and block.

    brennie brennie

    What is cmm? This needs a better name.

    brennie brennie

    Blank line between statement and block.

    brennie brennie

    Can you include some sample output of shellcheck?

    brennie brennie

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

    brennie brennie

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

    brennie brennie

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

    brennie brennie

    This should not be included in your change.

    brennie brennie
    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.