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.

Description From Last Updated

E999 SyntaxError: EOL while scanning string literal

reviewbotreviewbot

F841 local variable 'vet_output' is assigned to but never used

reviewbotreviewbot

F821 undefined name 'output'

reviewbotreviewbot

Single quotes.

brenniebrennie

No blank line here.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

No blank line here.

brenniebrennie

I think this is a typo because line.split(':', 1) will always two values if : is in line. Should this ...

brenniebrennie

This can all be one line.

daviddavid

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

daviddavid

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

daviddavid

Slightly better to do vet_args_list.append(path)

daviddavid

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

daviddavid

Add an additional blank line here.

daviddavid

The "note" is probably not necessary.

daviddavid

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

ilawilaw

Should this be using single quotes?

gojeffchogojeffcho

E202 whitespace before ']'

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