Add Go tool

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

Information

ReviewBot
master

Reviewers

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)
     
     
    Show all issues

    Single quotes.

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

    No blank line here.

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

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues

    No blank line here.

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

    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)
     
     
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

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

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

    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)
     
     
    Show all issues

    Slightly better to do vet_args_list.append(path)

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

    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)
     
     
    Show all issues

    Add an additional blank line here.

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

    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)
     
     
    Show all issues

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

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

    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
david
Review request changed

Status: Discarded

Loading...