Summary

Add Go tool

Review Request #10302 — Created Nov. 1, 2018 and discarded Dec. 2, 2020, 7:14 p.m.

Information

Owner

ammar

Repository

ReviewBot

Branch

master

Bugs

Depends On

~~10095~~

Reviewers

Groups

reviewbot, students

People

Description

This patch adds a Go Tool to Review Bot for static analysis of Go code. 

It makes use of the following two Go commands:


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

constructs



gofmt # formats the code

Testing Done

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

Issues

Description	From	Last Updated
E999 SyntaxError: EOL while scanning string literal	reviewbot	Nov. 1, 2018, 5:54 p.m.
F841 local variable 'vet_output' is assigned to but never used	reviewbot	Nov. 1, 2018, 6:11 p.m.
F821 undefined name 'output'	reviewbot	Nov. 1, 2018, 6:11 p.m.
Single quotes.	brennie	Nov. 10, 2018, 12:54 p.m.
No blank line here.	brennie	Nov. 10, 2018, 12:57 p.m.
We might want to make a note that we are using gofmt over go fmt because the former does not …	brennie	Nov. 10, 2018, 12:57 p.m.
In the case of a string that contains single quotes, it is more readable to use double quotes, e.g. "%s's …	brennie	Nov. 10, 2018, 12:58 p.m.
No blank line here.	brennie	Nov. 10, 2018, 12:58 p.m.
I think this is a typo because line.split(':', 1) will always two values if : is in line. Should this …	brennie	Nov. 10, 2018, 1:23 p.m.
This can all be one line.	david	Dec. 14, 2018, 10:52 a.m.
Instead of ignoring all errors, how about just extra_ignore_errors=(2,)	david	Nov. 27, 2018, 5:14 p.m.
This will put all of the "options" into a single argument. Do we want to use settings['vet_options'].split()?	david	Nov. 28, 2018, 7:21 p.m.
Slightly better to do vet_args_list.append(path)	david	Nov. 28, 2018, 7:21 p.m.
The number of = characters should line up with the number of characters in the title: ======= Go Tool =======	david	Nov. 28, 2018, 7:21 p.m.
Add an additional blank line here.	david	Nov. 28, 2018, 7:21 p.m.
The "note" is probably not necessary.	david	Nov. 28, 2018, 7:21 p.m.
The 'go' between run and go vet can be removed?	ilaw	Nov. 29, 2018, 7:44 p.m.
Should this be using single quotes?	gojeffcho	Nov. 29, 2018, 2:36 p.m.
E202 whitespace before ']'	reviewbot	Dec. 14, 2018, 10:52 a.m.

flake8 failed.

JSHint passed.

flake8

bot/reviewbot/tools/go.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E999 SyntaxError: EOL while scanning string literal
```

Commit:

0437f12a63e70c4f5451df2ac466e46de23e795a

32b2511d6d2f75bda3aece4e796e46f6f961c080

Diff:

Revision 2 (+92)

Show changes

bot/reviewbot/tools/go.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

bot/reviewbot/tools/go.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'vet_output' is assigned to but never used
```
bot/reviewbot/tools/go.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F821 undefined name 'output'
```

Commit:

32b2511d6d2f75bda3aece4e796e46f6f961c080

935d343aed4b1f6e4ed7a8944c7ac627d6329a90

Diff:

Revision 3 (+92)

Show changes

bot/reviewbot/tools/go.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

bot/reviewbot/tools/go.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Single quotes.
```
bot/reviewbot/tools/go.py (Diff revision 3)
The issue has been resolved. Show all issues
```
No blank line here.
```

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

The issue has been resolved. 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.

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

The issue has been resolved. 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"

bot/reviewbot/tools/go.py (Diff revision 3)
The issue has been resolved. Show all issues
```
No blank line here.
```

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

The issue has been resolved. 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.

Description:

~		Add Go tool
	~	This patch adds a Go Tool to Review Bot for static analysis of Go code.
	+	It makes use of the following two Go commands:
	+
	+
	+
	+	`go tool vet` # static code analysis to find possible bugs or suspicious constructs
	+
	+
	+
	+	`gofmt` # formats the code

Testing Done:

~		Add more support for the Go language. This patch adds a Go Tool class which
	~	Manual tests. Successfuly adds issues at the right line numbers.
-		runs two commands on go files:
-
-
-
-		1) go tool vet # static code analysis to find possible bugs or suspicious
-		constructs
-
-
-
-		2) gofmt # formats the code

Commit:

935d343aed4b1f6e4ed7a8944c7ac627d6329a90

8c4b72e4df9ece1a27c4eb2e7c89a9629b396b50

Diff:

Revision 4 (+123)

Show changes

	README.rst
	bot/README.rst
	bot/setup.py
	bot/reviewbot/tools/go.py
	docs/reviewbot/tools/gotool.rst
	docs/reviewbot/tools/index.rst
	extension/README.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:: [WIP] Add Go tool
Add Go tool

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

The issue has been resolved. Show all issues

This can all be one line.

ammar Nov. 28, 2018, 7:31 p.m.

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

bot/reviewbot/tools/go.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Instead of ignoring all errors, how about just extra_ignore_errors=(2,)
```

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

The issue has been resolved. Show all issues

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

ammar Nov. 28, 2018, 7:31 p.m.
```
Yes, that makes more sense.
```

bot/reviewbot/tools/go.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Slightly better to do vet_args_list.append(path)
```

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

The issue has been resolved. Show all issues

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

=======
Go Tool
=======

docs/reviewbot/tools/gotool.rst (Diff revision 4)
The issue has been resolved. Show all issues
```
Add an additional blank line here.
```
docs/reviewbot/tools/gotool.rst (Diff revision 4)
The issue has been resolved. Show all issues
```
The "note" is probably not necessary.
```

Commit:

8c4b72e4df9ece1a27c4eb2e7c89a9629b396b50

38c2f2b65e9482fabd5582f97f0c830a1aad633b

Diff:

Revision 5 (+123)

Show changes

	README.rst
	bot/README.rst
	bot/setup.py
	bot/reviewbot/tools/go.py
	docs/reviewbot/tools/gotool.rst
	docs/reviewbot/tools/index.rst
	extension/README.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The command is sometimes referred to as go tool vet and sometimes go vet - should it be one or the other?

bot/reviewbot/tools/go.py (Diff revision 5)
The issue has been resolved. Show all issues
```
The 'go' between run and go vet can be removed?
```

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

The issue has been resolved. Show all issues

Should this be using single quotes?

ammar Nov. 29, 2018, 2:36 p.m.

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

Commit:

38c2f2b65e9482fabd5582f97f0c830a1aad633b

aa2326d7df0c65f9d2f034b18fd8a1b3d1ddbce8

Diff:

Revision 6 (+123)

Show changes

	README.rst
	bot/README.rst
	bot/setup.py
	bot/reviewbot/tools/go.py
	docs/reviewbot/tools/gotool.rst
	docs/reviewbot/tools/index.rst
	extension/README.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

aa2326d7df0c65f9d2f034b18fd8a1b3d1ddbce8

156730edb71f2f8c74271c923b45c55ef25b4381

Diff:

Revision 7 (+119)

Show changes

	README.rst
	bot/README.rst
	bot/setup.py
	bot/reviewbot/tools/go.py
	docs/reviewbot/tools/gotool.rst
	docs/reviewbot/tools/index.rst
	extension/README.rst

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

bot/reviewbot/tools/go.py (Diff revision 7)
The issue has been resolved. Show all issues
```
E202 whitespace before ']'
```

Commit:

156730edb71f2f8c74271c923b45c55ef25b4381

ad55311ebd45a2d6aef0b3ac0af11cfbac662928

Diff:

Revision 8 (+119)

Show changes

	README.rst
	bot/README.rst
	bot/setup.py
	bot/reviewbot/tools/go.py
	docs/reviewbot/tools/gotool.rst
	docs/reviewbot/tools/index.rst
	extension/README.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Status:: Discarded