Skip to content

Commit

Permalink
Addressing missed feedback from #551 and returning FilePath (#553)
Browse files Browse the repository at this point in the history 
Returning `FilePath` instead of `File` for better integration with CI tooling. 

Also addressing feedback from #551.

Co-authored-by: Abhinav Gupta <[email protected]>
Co-authored-by: Sung Yoon Whang <[email protected]>
  • Loading branch information
@shirchen @abhinav @sywhang
3 people authored Dec 3, 2021
1 parent ab2e4a7 commit 944fb9f
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 39 deletions.
6 changes: 3 additions & 3 deletions cmd/thriftbreak/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func main() {
// readableOutput prints every lint error on a separate line.
func readableOutput(w io.Writer) func(compare.Diagnostic) error {
return func(diagnostic compare.Diagnostic) error {
if _, err := fmt.Fprintln(w, diagnostic.String()); err != nil {
if _, err := fmt.Fprintln(w, &diagnostic); err != nil {
return fmt.Errorf("failed to output a lint error: %v", err)
}

Expand All @@ -57,7 +57,7 @@ func jsonOutput(w io.Writer) func(compare.Diagnostic) error {
return func(diagnostic compare.Diagnostic) error {
// Encode adds a trailing newline.
if err := enc.Encode(diagnostic); err != nil {
return fmt.Errorf("failed to encode: %v", err)
return fmt.Errorf("encode as JSON: %v", err)
}

return nil
Expand All @@ -69,7 +69,7 @@ func run(args []string) error {
gitRepo := flag.String("C", "",
"location of git repository. Defaults to current directory.")
jsonOut := flag.Bool("json", false,
"output as a list of newline-delimited JSON objects with the following fields: File and Message")
"output as a list of newline-delimited JSON objects with the following fields: FilePath and Message")
if err := flag.Parse(args); err != nil {
return err
}
Expand Down
37 changes: 15 additions & 22 deletions cmd/thriftbreak/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ package main

import (
"bytes"
"fmt"
"io"
"io/ioutil"
"os"
Expand All @@ -36,10 +35,9 @@ import (

func TestThriftBreakIntegration(t *testing.T) {
tests := []struct {
desc string
want string
gitDirArg string
extraCmd string
desc string
want string
extraCmd string
}{
{
desc: "output",
Expand All @@ -48,17 +46,15 @@ func TestThriftBreakIntegration(t *testing.T) {
`v2.thrift:deleting service "Bar"` + "\n" +
`v1.thrift:removing method "methodA" in service "Foo"` + "\n" +
`v1.thrift:adding a required field "C" to "AddedRequiredField"` + "\n",
gitDirArg: "-C=%s",
},
{
desc: "json output",
want: `{"File":"c.thrift","Message":"deleting service \"Baz\""}` + "\n" +
`{"File":"d.thrift","Message":"deleting service \"Qux\""}` + "\n" +
`{"File":"v2.thrift","Message":"deleting service \"Bar\""}` + "\n" +
`{"File":"v1.thrift","Message":"removing method \"methodA\" in service \"Foo\""}` + "\n" +
`{"File":"v1.thrift","Message":"adding a required field \"C\" to \"AddedRequiredField\""}` + "\n",
gitDirArg: "-C=%s",
extraCmd: "--json",
want: `{"FilePath":"c.thrift","Message":"deleting service \"Baz\""}` + "\n" +
`{"FilePath":"d.thrift","Message":"deleting service \"Qux\""}` + "\n" +
`{"FilePath":"v2.thrift","Message":"deleting service \"Bar\""}` + "\n" +
`{"FilePath":"v1.thrift","Message":"removing method \"methodA\" in service \"Foo\""}` + "\n" +
`{"FilePath":"v1.thrift","Message":"adding a required field \"C\" to \"AddedRequiredField\""}` + "\n",
extraCmd: "--json",
},
}
from := map[string]string{
Expand Down Expand Up @@ -102,12 +98,9 @@ func TestThriftBreakIntegration(t *testing.T) {
}(os.Stdout)
os.Stdout = f

// Pinning not to run into scoping errors.
gitDir := tt.gitDirArg
extraCmd := tt.extraCmd
err = run([]string{fmt.Sprintf(gitDir, tmpDir), extraCmd})
err = run([]string{"-C=" + tmpDir, tt.extraCmd})

require.Error(t, err, "expected no errors")
require.Error(t, err, "expected an error with Thrift backwards incompatible changes")
assert.EqualError(t, err, "found 5 issues")

stderr, err := ioutil.ReadFile(f.Name())
Expand All @@ -119,7 +112,7 @@ func TestThriftBreakIntegration(t *testing.T) {
}
}

func TestJsonPrinter(t *testing.T) {
func TestDiagnosticPrinters(t *testing.T) {
t.Parallel()
tests := []struct {
desc string
Expand All @@ -128,7 +121,7 @@ func TestJsonPrinter(t *testing.T) {
}{
{
desc: "json writer",
want: "{\"File\":\"foo.thrift\",\"Message\":\"error\"}\n",
want: `{"FilePath":"foo.thrift","Message":"error"}` + "\n",
writer: jsonOutput,
},
{
Expand All @@ -144,8 +137,8 @@ func TestJsonPrinter(t *testing.T) {
var b bytes.Buffer
w := tt.writer(&b)
err := w(compare.Diagnostic{
File: "foo.thrift",
Message: "error",
FilePath: "foo.thrift",
Message: "error",
})
require.NoError(t, err)
assert.Equal(t, tt.want, b.String())
Expand Down
41 changes: 28 additions & 13 deletions internal/compare/compare.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,18 @@ import (

// Diagnostic is a message associated with an error and a file name.
type Diagnostic struct {
File string // File where error was discovered
Message string // Message contains error message.
FilePath string // FilePath where error was discovered.
Message string // Message contains error message.
}

func (d *Diagnostic) String() string {
return fmt.Sprintf("%s:%s", d.File, d.Message)
return fmt.Sprintf("%s:%s", d.FilePath, d.Message)
}

// Pass provides all reported errors.
type Pass struct {
lints []Diagnostic
lints []Diagnostic
GitDir string
}

// Report reports an error.
Expand Down Expand Up @@ -68,8 +69,9 @@ func (p *Pass) CompareModules(from, to *compile.Module) {
p.service(fromService, to.Services[name])
}

file := p.getRelativePath(from.ThriftPath)
for n, fromType := range from.Types {
p.typ(fromType, to.Types[n], filepath.Base(from.ThriftPath))
p.typ(fromType, to.Types[n], file)
}
}

Expand All @@ -89,7 +91,7 @@ func (p *Pass) requiredField(fromField, toField *compile.FieldSpec, to *compile.
fromRequired := fromField.Required
if !fromRequired && toField.Required {
p.Report(Diagnostic{
File: file,
FilePath: file,
Message: fmt.Sprintf(
"changing an optional field %q in %q to required",
toField.ThriftName(), to.ThriftName()),
Expand All @@ -109,7 +111,7 @@ func (p *Pass) structSpecs(from, to *compile.StructSpec, file string) {
p.requiredField(fromField, toField, to, file)
} else if toField.Required {
p.Report(Diagnostic{
File: file,
FilePath: file,
Message: fmt.Sprintf("adding a required field %q to %q",
toField.ThriftName(), to.ThriftName()),
})
Expand All @@ -121,22 +123,35 @@ func (p *Pass) service(from, to *compile.ServiceSpec) {
if to == nil {
// Service was deleted, which is not backwards compatible.
p.Report(Diagnostic{
File: filepath.Base(from.File), // toModule could have been deleted.
Message: fmt.Sprintf("deleting service %q", from.Name),
FilePath: filepath.Base(from.File), // toModule could have been deleted.
Message: fmt.Sprintf("deleting service %q", from.Name),
})

return
}
file := p.getRelativePath(from.File)
for n := range from.Functions {
p.function(to.Functions[n], n, filepath.Base(from.File), from.Name)
p.function(to.Functions[n], n, file, from.Name)
}
}

func (p *Pass) function(to *compile.FunctionSpec, fn string, file string, service string) {
// getRelativePath returns a relative path to a file or
// fallbacks to file name for cases when it was deleted.
func (p *Pass) getRelativePath(filePath string) string {
if file, err := filepath.Rel(p.GitDir, filePath); err == nil {
return file
}
// If a file was deleted, then we will not be able to
// find a relative path to it.
return filepath.Base(filePath)
}

func (p *Pass) function(to *compile.FunctionSpec, fn string, path string, service string) {
file := p.getRelativePath(path)
if to == nil {
p.Report(Diagnostic{
File: file,
Message: fmt.Sprintf("removing method %q in service %q", fn, service),
FilePath: file,
Message: fmt.Sprintf("removing method %q in service %q", fn, service),
})
}
}
31 changes: 31 additions & 0 deletions internal/compare/compare_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,3 +215,34 @@ func TestServicesError(t *testing.T) {
})
}
}

func TestRelativePath(t *testing.T) {
t.Parallel()
tests := []struct {
desc string
path string
want string
}{
{
desc: "good case",
path: "/home/foo/bar/buz/buz.go",
want: "buz/buz.go",
},
{
desc: "fallback case where we just return file name",
path: "foo/buz.go",
want: "buz.go",
},
}
for _, tt := range tests {
tt := tt
t.Run(tt.desc, func(t *testing.T) {
t.Parallel()
p := Pass{
GitDir: "/home/foo/bar",
}
assert.Equal(t, tt.want, p.getRelativePath(tt.path))
})
}

}
4 changes: 3 additions & 1 deletion internal/git/git.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ func NewGitFS(gitDir string, repo *git.Repository, tree *object.Tree) *FS {
// Compare takes a path to a git repository and returns errors between HEAD and HEAD~
// for any incompatible Thrift changes between the two shas.
func Compare(path string) (compare.Pass, error) {
var pass compare.Pass
pass := compare.Pass{
GitDir: path,
}
r, err := git.PlainOpenWithOptions(path, &git.PlainOpenOptions{
DetectDotGit: true,
EnableDotGitCommonDir: true,
Expand Down

0 comments on commit 944fb9f

Please sign in to comment.