Repository: alecthomas/gometalinter
Pull request #394 (2017-11-23 14:44:39)

Added config option to output absolute paths
Re: #393 

dnephin: I think the feature is very reasonable, but I don't like adding a new config option and flag for this.

We already have` --format` (and `Format` in the config) to set the output format. How about we add support for `{{ .Path.Abs }}` to the format? 

This will likely be a slightly larger change. Instead of making the path relative in `processOutput()` the path would need to be made absolute.

One way to handle this would be a new `type issuePath struct { root string; path string }` to store both the root and the relative path from the root. Then `issuePath.String()` can return a relative path so any existing formats will be backwards compatible, and `issuePath.Abs()` can return the absolute path.

It will mean that any place `issue.Path()` is currently used may need to be updated, but that is probably a good thing so that we don't accidentally break something that assumes it would receive a relative path.
deltaskelta: > It will mean that any place issue.Path() is currently used may need to be updated, but that is probably a good thing so that we don't accidentally break something that assumes it would receive a relative path.

Do you mean to say any place that `issue.String()` is currently used? I was grepping around and couldn't find any `issue.Path()`
dnephin: Oops, `issue.Path` is a field not a method. Sorry, I should have said `issue.Path`. 
deltaskelta: I have a working fix here but there are two issues...

[] IDK where or how to document the option in the `--help` text.
[] There are now some annoying warnings when the tests are run because the file paths that are put into the tests are not there...I guess I can make the test look for actual files (like main.go)?
deltaskelta: The initial idea caused deep problems in the regression tests since it changed the underlying structure of `Issue`. 

Since the absolute path is a necessary calculation when figuring the relative path, I instead added an `AbsPath` field to the `Issue` which should maintain the current functionality as well as the ability to call `{{.AbsPath}}` with `--format`
dnephin: I'd like to avoid adding an `AbsPath` field to `Issue`. Storing the same value twice can lead to bugs. In this case it's not the same value, but there's a lot of overlap.

The regression test failures were because the issuePath was not being marshalled properly. Adding this fixed it:

```go
func (i issuePath) MarshalJSON() ([]byte, error) {
	return json.Marshal(i.String())
}
```

I pushed a commit which adds this method, and a couple constructors for `issuePath`.
deltaskelta: It looks good in the code, but when I try to call for method with 

```--format="{{.Path.Abs}}:{{.Line}}:{{if .Col}}{{.Col}}{{end}}:{{.Severity}}: {{.Message}} ({{.Linter}})"``` 

I get an error...

```gometalinter: error: Invalid output format: template: output:1:7: executing "output" at <.Path.Abs>: can't evaluate field Abs in type string
```

Am I missing something?
deltaskelta: Should we document anywhere that this can be done? It would be rather hard for someone to derive by looking through the code. I would have certainly missed it
dnephin: Ya, I think that would be good. Right now all the docs are in the README, so I guess a section about `Format` would be good, under the `Configuration File` section. It can mention that `--format` also works.
deltaskelta: @alecthomas @dnephin is there anything else that needs to be done to get this merged? I have a pending PR w0rp/ale#394 repo that could really use this!
dnephin: I wanted to give @alecthomas a chance to look before merging
alecthomas: Thank you, this is great!

Patch:
diff --git a/README.md b/README.md
index 809a60a..a9c902f 100644
--- a/README.md
+++ b/README.md
@@ -110,6 +110,21 @@ Here is an example configuration file:
 }
 ```
 
+#### `Format` key
+
+The default `Format` key places the different fields of an `Issue` into a template. this
+corresponds to the `--format` option command-line flag.
+
+Default `Format`:
+```
+Format: "{{.Path}}:{{.Line}}:{{if .Col}}{{.Col}}{{end}}:{{.Severity}}: {{.Message}} ({{.Linter}})"
+```
+
+#### Format Methods
+
+* `{{.Path.Relative}}` - equivalent to `{{.Path}}` which outputs a relative path to the file
+* `{{.Path.Abs}}` - outputs an absolute path to the file
+
 ### Adding Custom linters
 
 Linters can be added and customized from the config file using the `Linters` field.
diff --git a/aggregate.go b/aggregate.go
index f4b44e4..1017891 100644
--- a/aggregate.go
+++ b/aggregate.go
@@ -25,7 +25,7 @@ func AggregateIssueChan(issues chan *Issue) chan *Issue {
 	go func() {
 		for issue := range issues {
 			key := issueKey{
-				path:    issue.Path,
+				path:    issue.Path.String(),
 				line:    issue.Line,
 				col:     issue.Col,
 				message: issue.Message,
diff --git a/checkstyle.go b/checkstyle.go
index 5122604..52ff239 100644
--- a/checkstyle.go
+++ b/checkstyle.go
@@ -33,14 +33,13 @@ func outputToCheckstyle(issues chan *Issue) int {
 	}
 	status := 0
 	for issue := range issues {
-		if lastFile != nil && lastFile.Name != issue.Path {
+		path := issue.Path.Relative()
+		if lastFile != nil && lastFile.Name != path {
 			out.Files = append(out.Files, lastFile)
 			lastFile = nil
 		}
 		if lastFile == nil {
-			lastFile = &checkstyleFile{
-				Name: issue.Path,
-			}
+			lastFile = &checkstyleFile{Name: path}
 		}
 
 		if config.Errors && issue.Severity != Error {
diff --git a/directives.go b/directives.go
index 10f2903..45d322e 100644
--- a/directives.go
+++ b/directives.go
@@ -5,6 +5,7 @@ import (
 	"go/ast"
 	"go/parser"
 	"go/token"
+	"os"
 	"sort"
 	"strings"
 	"sync"
@@ -67,11 +68,12 @@ func newDirectiveParser() *directiveParser {
 // IsIgnored returns true if the given linter issue is ignored by a linter directive.
 func (d *directiveParser) IsIgnored(issue *Issue) bool {
 	d.lock.Lock()
-	ranges, ok := d.files[issue.Path]
+	path := issue.Path.Relative()
+	ranges, ok := d.files[path]
 	if !ok {
-		ranges = d.parseFile(issue.Path)
+		ranges = d.parseFile(path)
 		sort.Sort(ranges)
-		d.files[issue.Path] = ranges
+		d.files[path] = ranges
 	}
 	d.lock.Unlock()
 	for _, r := range ranges {
@@ -204,10 +206,16 @@ func filterIssuesViaDirectives(directives *directiveParser, issues chan *Issue)
 
 func warnOnUnusedDirective(directives *directiveParser) []*Issue {
 	out := []*Issue{}
+
+	cwd, err := os.Getwd()
+	if err != nil {
+		warning("failed to get working directory %s", err)
+	}
+
 	for path, ranges := range directives.Unmatched() {
 		for _, ignore := range ranges {
 			issue, _ := NewIssue("nolint", config.formatTemplate)
-			issue.Path = path
+			issue.Path = newIssuePath(cwd, path)
 			issue.Line = ignore.start
 			issue.Col = ignore.col
 			issue.Message = "nolint directive did not match any issue"
diff --git a/execute.go b/execute.go
index 1247697..da9eb5f 100644
--- a/execute.go
+++ b/execute.go
@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"reflect"
 	"regexp"
 	"strconv"
@@ -238,8 +237,10 @@ func processOutput(dbg debugFunction, state *linterState, out []byte) {
 			}
 			switch name {
 			case "path":
-				issue.Path = relativePath(cwd, part)
-
+				issue.Path, err = newIssuePathFromAbsPath(cwd, part)
+				if err != nil {
+					warning("failed to make %s a relative path: %s", part, err)
+				}
 			case "line":
 				n, err := strconv.ParseInt(part, 10, 32)
 				kingpin.FatalIfError(err, "line matched invalid integer")
@@ -274,37 +275,6 @@ func processOutput(dbg debugFunction, state *linterState, out []byte) {
 	}
 }
 
-func relativePath(root, path string) string {
-	fallback := path
-	root = resolvePath(root)
-	path = resolvePath(path)
-	var err error
-	path, err = filepath.Rel(root, path)
-	if err != nil {
-		warning("failed to make %s a relative path: %s", fallback, err)
-		return fallback
-	}
-	return path
-}
-
-func resolvePath(path string) string {
-	var err error
-	fallback := path
-	if !filepath.IsAbs(path) {
-		path, err = filepath.Abs(path)
-		if err != nil {
-			warning("failed to make %s an absolute path: %s", fallback, err)
-			return fallback
-		}
-	}
-	path, err = filepath.EvalSymlinks(path)
-	if err != nil {
-		warning("failed to resolve symlinks in %s: %s", fallback, err)
-		return fallback
-	}
-	return path
-}
-
 func maybeSortIssues(issues chan *Issue) chan *Issue {
 	if reflect.DeepEqual([]string{"none"}, config.Sort) {
 		return issues
diff --git a/issue.go b/issue.go
index 982f450..46c609a 100644
--- a/issue.go
+++ b/issue.go
@@ -2,8 +2,10 @@ package main
 
 import (
 	"bytes"
+	"encoding/json"
 	"fmt"
 	"io/ioutil"
+	"path/filepath"
 	"sort"
 	"strings"
 	"text/template"
@@ -21,13 +23,59 @@ const (
 	Warning Severity = "warning"
 )
 
+type IssuePath struct {
+	root string
+	path string
+}
+
+func (i IssuePath) String() string {
+	return i.Relative()
+}
+
+func (i IssuePath) Relative() string {
+	return i.path
+}
+
+func (i IssuePath) Abs() string {
+	return filepath.Join(i.root, i.path)
+}
+
+func (i IssuePath) MarshalJSON() ([]byte, error) {
+	return json.Marshal(i.String())
+}
+
+func newIssuePath(root, path string) IssuePath {
+	return IssuePath{root: root, path: path}
+}
+
+// newIssuePathFromAbsPath returns a new issuePath from a path that may be
+// an absolute path. root must be an absolute path.
+func newIssuePathFromAbsPath(root, path string) (IssuePath, error) {
+	resolvedRoot, err := filepath.EvalSymlinks(root)
+	if err != nil {
+		return newIssuePath(root, path), err
+	}
+
+	resolvedPath, err := filepath.EvalSymlinks(path)
+	if err != nil {
+		return newIssuePath(root, path), err
+	}
+
+	if !filepath.IsAbs(path) {
+		return newIssuePath(resolvedRoot, resolvedPath), nil
+	}
+
+	relPath, err := filepath.Rel(resolvedRoot, resolvedPath)
+	return newIssuePath(resolvedRoot, relPath), err
+}
+
 type Issue struct {
-	Linter     string   `json:"linter"`
-	Severity   Severity `json:"severity"`
-	Path       string   `json:"path"`
-	Line       int      `json:"line"`
-	Col        int      `json:"col"`
-	Message    string   `json:"message"`
+	Linter     string    `json:"linter"`
+	Severity   Severity  `json:"severity"`
+	Path       IssuePath `json:"path"`
+	Line       int       `json:"line"`
+	Col        int       `json:"col"`
+	Message    string    `json:"message"`
 	formatTmpl *template.Template
 }
 
@@ -50,7 +98,11 @@ func (i *Issue) String() string {
 		if i.Col != 0 {
 			col = fmt.Sprintf("%d", i.Col)
 		}
-		return fmt.Sprintf("%s:%d:%s:%s: %s (%s)", strings.TrimSpace(i.Path), i.Line, col, i.Severity, strings.TrimSpace(i.Message), i.Linter)
+		return fmt.Sprintf("%s:%d:%s:%s: %s (%s)",
+			strings.TrimSpace(i.Path.Relative()),
+			i.Line, col, i.Severity,
+			strings.TrimSpace(i.Message),
+			i.Linter)
 	}
 	buf := new(bytes.Buffer)
 	_ = i.formatTmpl.Execute(buf, i)
@@ -76,7 +128,7 @@ func CompareIssue(l, r Issue, order []string) bool {
 	for _, key := range order {
 		switch {
 		case key == "path" && l.Path != r.Path:
-			return l.Path < r.Path
+			return l.Path.String() < r.Path.String()
 		case key == "line" && l.Line != r.Line:
 			return l.Line < r.Line
 		case key == "column" && l.Col != r.Col:


Test patch:
diff --git a/issue_test.go b/issue_test.go
index 2d456d8..3a027b5 100644
--- a/issue_test.go
+++ b/issue_test.go
@@ -10,10 +10,10 @@ import (
 
 func TestSortedIssues(t *testing.T) {
 	actual := []*Issue{
-		{Path: "b.go", Line: 5, Col: 1},
-		{Path: "a.go", Line: 3, Col: 2},
-		{Path: "b.go", Line: 1, Col: 3},
-		{Path: "a.go", Line: 1, Col: 4},
+		{Path: newIssuePath("", "b.go"), Line: 5, Col: 1},
+		{Path: newIssuePath("", "a.go"), Line: 3, Col: 2},
+		{Path: newIssuePath("", "b.go"), Line: 1, Col: 3},
+		{Path: newIssuePath("", "a.go"), Line: 1, Col: 4},
 	}
 	issues := &sortedIssues{
 		issues: actual,
@@ -21,18 +21,18 @@ func TestSortedIssues(t *testing.T) {
 	}
 	sort.Sort(issues)
 	expected := []*Issue{
-		{Path: "a.go", Line: 1, Col: 4},
-		{Path: "a.go", Line: 3, Col: 2},
-		{Path: "b.go", Line: 1, Col: 3},
-		{Path: "b.go", Line: 5, Col: 1},
+		{Path: newIssuePath("", "a.go"), Line: 1, Col: 4},
+		{Path: newIssuePath("", "a.go"), Line: 3, Col: 2},
+		{Path: newIssuePath( "", "b.go"), Line: 1, Col: 3},
+		{Path: newIssuePath( "", "b.go"), Line: 5, Col: 1},
 	}
 	require.Equal(t, expected, actual)
 }
 
 func TestCompareOrderWithMessage(t *testing.T) {
 	order := []string{"path", "line", "column", "message"}
-	issueM := Issue{Path: "file.go", Message: "message"}
-	issueU := Issue{Path: "file.go", Message: "unknown"}
+	issueM := Issue{Path: newIssuePath("", "file.go"), Message: "message"}
+	issueU := Issue{Path: newIssuePath("", "file.go"), Message: "unknown"}
 
 	assert.True(t, CompareIssue(issueM, issueU, order))
 	assert.False(t, CompareIssue(issueU, issueM, order))