Add Go tool

Review Request #10302 — Created Nov. 1, 2018 and updated

ammar
ReviewBot
master
10095
ad55311...
reviewbot, students

This patch adds a Go Tool to Review Bot for static analysis of Go code.
It makes use of the following two Go commands:

  1. go tool vet # static code analysis to find possible bugs or suspicious
    constructs

  1. gofmt # formats the code

Manual tests. Successfuly adds issues at the right line numbers.

  • 0
  • 0
  • 19
  • 0
  • 19
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

ammar
Review request changed

Commit:

-0437f12a63e70c4f5451df2ac466e46de23e795a
+32b2511d6d2f75bda3aece4e796e46f6f961c080

Diff:

Revision 2 (+92)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ammar
brennie
  1. 
      
  2. bot/reviewbot/tools/go.py (Diff revision 3)
     
     

    Single quotes.

  3. bot/reviewbot/tools/go.py (Diff revision 3)
     
     

    No blank line here.

  4. bot/reviewbot/tools/go.py (Diff revision 3)
     
     

    We might want to make a note that we are using gofmt over go fmt because the former does not allow us to specify options that we want to use.

  5. bot/reviewbot/tools/go.py (Diff revision 3)
     
     

    In the case of a string that contains single quotes, it is more readable to use double quotes, e.g.

    "%s's formatting differs from gofmt's"

  6. bot/reviewbot/tools/go.py (Diff revision 3)
     
     

    No blank line here.

  7. bot/reviewbot/tools/go.py (Diff revision 3)
     
     

    I think this is a typo because line.split(':', 1) will always two values if : is in line. Should this say If split does not return two values?

    Also this should be in the except block.

    Comments must be proper sentences.

  8. 
      
ammar
ammar
david
  1. 
      
  2. bot/reviewbot/tools/go.py (Diff revision 4)
     
     
     
     
     
     

    This can all be one line.

    1. This is the same pattern as in other tools (flake8, cpplint etc). Would you still like to change it?

  3. bot/reviewbot/tools/go.py (Diff revision 4)
     
     

    Instead of ignoring all errors, how about just extra_ignore_errors=(2,)

  4. bot/reviewbot/tools/go.py (Diff revision 4)
     
     

    This will put all of the "options" into a single argument. Do we want to use settings['vet_options'].split()?

    1. Yes, that makes more sense.

  5. bot/reviewbot/tools/go.py (Diff revision 4)
     
     

    Slightly better to do vet_args_list.append(path)

  6. docs/reviewbot/tools/gotool.rst (Diff revision 4)
     
     
     

    The number of = characters should line up with the number of characters in the title:

    =======
    Go Tool
    =======
    
  7. docs/reviewbot/tools/gotool.rst (Diff revision 4)
     
     

    Add an additional blank line here.

  8. docs/reviewbot/tools/gotool.rst (Diff revision 4)
     
     
     

    The "note" is probably not necessary.

  9. 
      
ammar
ilaw
  1. 
      
  2. The command is sometimes referred to as go tool vet and sometimes go vet - should it be one or the other?

  3. bot/reviewbot/tools/go.py (Diff revision 5)
     
     

    The 'go' between run and go vet can be removed?

  4. 
      
gojeffcho
  1. 
      
  2. bot/reviewbot/tools/go.py (Diff revision 5)
     
     

    Should this be using single quotes?

    1. Singles quotes are used inside the string, so we decided in a previous issue to do it this way.

  3. 
      
ammar
ammar
Review request changed

Commit:

-aa2326d7df0c65f9d2f034b18fd8a1b3d1ddbce8
+156730edb71f2f8c74271c923b45c55ef25b4381

Diff:

Revision 7 (+119)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ammar
Review request changed

Commit:

-156730edb71f2f8c74271c923b45c55ef25b4381
+ad55311ebd45a2d6aef0b3ac0af11cfbac662928

Diff:

Revision 8 (+119)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...