• 
      

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