Page MenuHomePhabricator
Differential D12120 Diff 30105 src/lint/linter/ArcanistGoVetLinter.php

Changeset View

Standalone View

View Options

src/lint/linter/ArcanistGoVetLinter.php

  • This file was added.
<?php
final class ArcanistGoVetLinter extends ArcanistExternalLinter {
public function getInfoName() {
return 'Go vet';
}
joshuaspenceUnsubmitted
Done
Inline Actions

Maybe Go Vet

joshuaspence: Maybe `Go Vet`
public function getInfoURI() {
return 'https://godoc.org/golang.org/x/tools/cmd/vet';
}
public function getInfoDescription() {
return pht(
'Vet examines Go source code and reports suspicious constructs.');
}
public function getLinterName() {
return 'GOVET';
}
public function getLinterConfigurationName() {
return 'govet';
}
public function getDefaultBinary() {
$binary = 'go';
if (Filesystem::binaryExists($binary)) {
// Vet is only accessible through 'go vet' or 'go tool vet'
// Let's manually try to find out if it's installed.
list($err, $stdout, $stderr) = exec_manual('go tool vet');
if ($err === 3) {
throw new ArcanistMissingLinterException(
sprintf(
"%s\n%s",
pht(
'Unable to locate "go vet" to run linter %s. You may need '.
'to install the binary, or adjust your linter configuration.',
get_class($this)),
pht(
'TO INSTALL: %s',
$this->getInstallInstructions())));
}
}
return $binary;
}
joshuaspenceUnsubmitted
Not Done
Inline Actions

There's no nice way of handling this... I'd just return go as the default binary.

joshuaspence: There's no nice way of handling this... I'd just return `go` as the default binary.
eMxyzptlkAuthorUnsubmitted
Not Done
Inline Actions

However the presence of the Go binary does not indicate if GoVet is installed or not. Unfortunately, ArcanistExternalLinter::checkBinaryConfiguration is final so I can't overload it.

eMxyzptlk: However the presence of the Go binary does not indicate if GoVet is installed or not.
joshuaspenceUnsubmitted
Not Done
Inline Actions

I installed golang-go on Ubuntu 15 (via apt-get) and it included go tool vet... when would it not be bundled? Is it common for it to not be bundled?

In any case, what would happen if getDefaultBinary() returned go and go was installed but not go tool vet? How would it fail?

joshuaspence: I installed `golang-go` on Ubuntu 15 (via `apt-get`) and it included `go tool vet`... when…
eMxyzptlkAuthorUnsubmitted
Not Done
Inline Actions

Perhaps it is included by default for Ubuntu (or whichever distro you're on) but AFAIK it is not bundled with the official distribution. If it's not found go tool vet ... will fail with an error message and an exit code 3

$ go tool vet /tmp/test.go; echo $?
go tool: no such tool "vet"; to install:
        go get golang.org/x/tools/cmd/vet
3

The actual linter will not fail but it will print an error for every file:

   Warning  (GOVET)


    >>>        1 package main
               2
               3 import (
               4        "flag"
>>> Lint for model/source.go:

Not sure where the 'package main' came from, it should be model, but in any case, it fails in an unwanted behavior.

eMxyzptlk: Perhaps it is included by default for Ubuntu (or whichever distro you're on) but AFAIK it is…
public function getInstallInstructions() {
return pht('Install Go vet using `go get golang.org/x/tools/cmd/vet`.');
}
joshuaspenceUnsubmitted
Done
Inline Actions

This would be better written as pht('Install Go vet using `%s`.', 'go get golang.org/x/tools/cmd/vet').

joshuaspence: This would be better written as `pht('Install Go vet using ```%s```.', 'go get golang.
public function shouldExpectCommandErrors() {
return true;
}
protected function canCustomizeLintSeverities() {
return true;
}
joshuaspenceUnsubmitted
Done
Inline Actions

These are the default values, just omit these methods.

joshuaspence: These are the default values, just omit these methods.
protected function getMandatoryFlags() {
return ['tool', 'vet'];
}
joshuaspenceUnsubmitted
Done
Inline Actions

This is PHP 5.4 syntax but we need to be compatible with PHP 5.2, so you'll need to change this to array('tool', 'vet')

joshuaspence: This is PHP 5.4 syntax but we need to be compatible with PHP 5.2, so you'll need to change this…
protected function parseLinterOutput($path, $err, $stdout, $stderr) {
$lines = phutil_split_lines($stderr, false);
$messages = array();
foreach ($lines as $line) {
$matches = explode(':', $line, 3);
if (count($matches) === 3) {
$message = new ArcanistLintMessage();
$message->setPath($path);
$message->setLine($matches[1]);
$message->setCode($this->getLinterName());
$message->setDescription(ucfirst(trim($matches[2])));
$message->setSeverity(ArcanistLintSeverity::SEVERITY_WARNING);
joshuaspenceUnsubmitted
Not Done
Inline Actions

You should maybe use $this->getLintMessageSeverity($code) here to allow the severity to be configurable.

joshuaspence: You should maybe use `$this->getLintMessageSeverity($code)` here to allow the severity to be…
eMxyzptlkAuthorUnsubmitted
Not Done
Inline Actions

So GoVet does not report errors, it only report a possibility of a problem, for instance, running go tool vet on the include test src/lint/linter/__tests__/govet/fmt.lint-test reports /tmp/test.go:6: possible formatting directive in Println call which may or may not be a real problem.

Given that, I think we will never report errors but only warnings. Should I still make it configurable?

eMxyzptlk: So GoVet does not report errors, it only report a possibility of a problem, for instance…
joshuaspenceUnsubmitted
Not Done
Inline Actions

I think it should be configurable. What you think is a warning, others may say is advice.

joshuaspence: I think it should be configurable. What you think is a warning, others may say is advice.
eMxyzptlkAuthorUnsubmitted
Not Done
Inline Actions

Alright I'll get it done tonight and update the differential.

eMxyzptlk: Alright I'll get it done tonight and update the differential.
$messages[] = $message;
}
}
return $messages;
}
}
Phabricator · Built by Phacility