From eb1d2613a8e7fc40b303e5960aecc827dcc496bd Mon Sep 17 00:00:00 2001 From: Yoan Blanc Date: Tue, 19 Nov 2019 08:24:06 +0000 Subject: [PATCH] git: refactor for tests Signed-off-by: Yoan Blanc --- .golangci.yml | 18 ++++- .goreleaser.yml | 1 - files.go | 75 +++++++++++++++++ files_test.go | 128 +++++++++++++++++++++++++++++ git.go | 25 ------ git_test.go | 31 ------- lint.go | 21 ++--- main.go | 196 +++++++++------------------------------------ print.go | 105 ++++++++++++++++++++++++ validators_test.go | 2 +- 10 files changed, 373 insertions(+), 229 deletions(-) create mode 100644 files.go create mode 100644 files_test.go delete mode 100644 git.go delete mode 100644 git_test.go create mode 100644 print.go diff --git a/.golangci.yml b/.golangci.yml index 2e04318..ede0786 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -7,21 +7,31 @@ linters-settings: linters: enable: - deadcode + - depguard + - dogsled - dupl - errcheck + - funlen + - gochecknoglobals + - gochecknoinits + - goconst - gocritic - gocyclo - goimports - golint - - gosimple - - govet + - gosec - ineffassign + - interfacer + - lll + - maligned - megacheck + - misspell - nakedret + - prealloc - scopelint - - staticcheck - structcheck + - unconvert - unused - varcheck - - gosec + - whitespace disable-all: true diff --git a/.goreleaser.yml b/.goreleaser.yml index a0afa5d..56713be 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -21,7 +21,6 @@ dockers: - image_templates: - greut/eclint:latest - greut/eclint:{{ .Tag }} - - greut/eclint:{{ .Tag }} - greut/eclint:v{{ .Major }} goos: linux goarch: amd64 diff --git a/files.go b/files.go new file mode 100644 index 0000000..ba067cf --- /dev/null +++ b/files.go @@ -0,0 +1,75 @@ +package main + +import ( + "bytes" + "os" + "os/exec" + "path/filepath" + + "github.com/go-logr/logr" +) + +// listFiles returns the list of files based on the input. +// +// When its empty, it relies on `git ls-files` first, which +// whould fail if `git` is not present or the current working +// directory is not managed by it. In that case, it work the +// current working directory. +// +// When args are given, it recursively walks into them. +func listFiles(log logr.Logger, args ...string) ([]string, error) { + if len(args) == 0 { + fs, err := gitLsFiles(log, ".") + if err == nil { + return fs, nil + } + + log.Error(err, "git ls-files failure") + args = append(args, ".") + } + + return walk(log, args...) +} + +// walk iterates on each path item recursively. +func walk(log logr.Logger, paths ...string) ([]string, error) { + files := make([]string, 0) + for _, path := range paths { + err := filepath.Walk(path, func(p string, i os.FileInfo, e error) error { + if e != nil { + return e + } + mode := i.Mode() + if mode.IsRegular() && !mode.IsDir() { + log.V(4).Info("index %s", p) + files = append(files, p) + } + return nil + }) + if err != nil { + return files, err + } + } + return files, nil +} + +// gitLsFiles returns the list of file based on what is in the git index. +// +// -z is mandatory as some repositories non-ASCII file names which creates +// quoted and escaped file names. +func gitLsFiles(log logr.Logger, path string) ([]string, error) { + output, err := exec.Command("git", "ls-files", "-z", path).Output() + if err != nil { + return nil, err + } + + fs := bytes.Split(output, []byte{0}) + // last line is empty + files := make([]string, len(fs)-1) + for i := 0; i < len(files); i++ { + p := string(fs[i]) + log.V(4).Info("index %s", p) + files[i] = p + } + return files, nil +} diff --git a/files_test.go b/files_test.go new file mode 100644 index 0000000..8470da5 --- /dev/null +++ b/files_test.go @@ -0,0 +1,128 @@ +package main + +import ( + "fmt" + "os" + "testing" + + tlogr "github.com/go-logr/logr/testing" +) + +const ( + // testdataSimple contains a sample editorconfig directory with + // some errors. + testdataSimple = "testdata/simple" +) + +func TestListFiles(t *testing.T) { + l := tlogr.TestLogger{} + d := testdataSimple + fs, err := listFiles(l, d) + if err != nil { + t.Fatal(err) + } + if len(fs) != 2 { + t.Errorf("%s should have two files, got %d", d, len(fs)) + } +} + +func TestListFilesNoArgs(t *testing.T) { + l := tlogr.TestLogger{} + d := testdataSimple + + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + defer func() { + if err := os.Chdir(cwd); err != nil { + t.Fatal(err) + } + }() + + err = os.Chdir(d) + if err != nil { + t.Fatal(err) + } + + fs, err := listFiles(l) + if err != nil { + t.Fatal(err) + } + if len(fs) != 2 { + t.Errorf("%s should have two files, got %d", d, len(fs)) + } +} + +func TestListFilesNoGit(t *testing.T) { + // FIXME... should be the null logger, right? + l := tlogr.NullLogger{} + d := fmt.Sprintf("/tmp/eclint/%d", os.Getpid()) + + err := os.MkdirAll(d, 0700) + if err != nil { + t.Fatal(err) + } + + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + defer func() { + if err := os.Chdir(cwd); err != nil { + t.Fatal(err) + } + }() + err = os.Chdir(d) + if err != nil { + t.Fatal(err) + } + + fs, err := listFiles(l) + if err != nil { + t.Fatal(err) + } + if len(fs) != 0 { + t.Errorf("%s should have two files, got %d", d, len(fs)) + } +} + +func TestWalk(t *testing.T) { + l := tlogr.TestLogger{} + d := testdataSimple + fs, err := walk(l, d) + if err != nil { + t.Fatal(err) + } + if len(fs) != 2 { + t.Errorf("%s should have two files, got %d", d, len(fs)) + } +} + +func TestGitLsFiles(t *testing.T) { + l := tlogr.TestLogger{} + d := testdataSimple + + fs, err := gitLsFiles(l, d) + if err != nil { + t.Fatal(err) + } + if len(fs) != 2 { + t.Errorf("%s should have two files, got %d", d, len(fs)) + } +} + +func TestGitLsFilesFailure(t *testing.T) { + l := tlogr.TestLogger{} + d := fmt.Sprintf("/tmp/eclint/%d", os.Getpid()) + + err := os.MkdirAll(d, 0700) + if err != nil { + t.Fatal(err) + } + + _, err = gitLsFiles(l, d) + if err == nil { + t.Error("an error was expected") + } +} diff --git a/git.go b/git.go deleted file mode 100644 index 208e83b..0000000 --- a/git.go +++ /dev/null @@ -1,25 +0,0 @@ -package main - -import ( - "bytes" - "os/exec" -) - -// gitLsFiles returns the list of file based on what is in the git index. -// -// -z is mandatory as some repositories non-ASCII file names which creates -// quoted and escaped file names. -func gitLsFiles(path string) ([]string, error) { - output, err := exec.Command("git", "ls-files", "-z", path).Output() - if err != nil { - return nil, err - } - - fs := bytes.Split(output, []byte{0}) - // last line is empty - files := make([]string, len(fs)-1) - for i := 0; i < len(files); i++ { - files[i] = string(fs[i]) - } - return files, nil -} diff --git a/git_test.go b/git_test.go deleted file mode 100644 index e53e568..0000000 --- a/git_test.go +++ /dev/null @@ -1,31 +0,0 @@ -package main - -import ( - "fmt" - "os" - "testing" -) - -func TestGitLsFiles(t *testing.T) { - d := "testdata/simple" - fs, err := gitLsFiles(d) - if err != nil { - t.Fatal(err) - } - if len(fs) != 2 { - t.Errorf("%s should have two files, got %d", d, len(fs)) - } -} - -func TestGitLsFilesFailure(t *testing.T) { - d := fmt.Sprintf("/tmp/eclint/%d", os.Getpid()) - err := os.MkdirAll(d, 0700) - if err != nil { - t.Fatal(err) - } - - _, err = gitLsFiles(d) - if err == nil { - t.Error("an error was expected") - } -} diff --git a/lint.go b/lint.go index ff13f93..303ec57 100644 --- a/lint.go +++ b/lint.go @@ -15,8 +15,11 @@ import ( // DefaultTabWidth sets the width of a tab used when counting the line length const DefaultTabWidth = 8 +// UnsetValue is the value equivalent to an empty / unset one. +const UnsetValue = "unset" + // validate is where the validations rules are applied -func validate(r io.Reader, log logr.Logger, def *editorconfig.Definition) []error { //nolint:gocyclo +func validate(r io.Reader, log logr.Logger, def *editorconfig.Definition) []error { //nolint:funlen,gocyclo var buf *bytes.Buffer // chardet uses a 8192 bytebuf for detection bufSize := 8192 @@ -30,17 +33,17 @@ func validate(r io.Reader, log logr.Logger, def *editorconfig.Definition) []erro var blockCommentStart []byte var blockComment []byte var blockCommentEnd []byte - if def.IndentStyle != "" && def.IndentStyle != "unset" { + if def.IndentStyle != "" && def.IndentStyle != UnsetValue { bs, ok := def.Raw["block_comment_start"] - if ok && bs != "" && bs != "unset" { + if ok && bs != "" && bs != UnsetValue { blockCommentStart = []byte(bs) bc, ok := def.Raw["block_comment"] - if ok && bc != "" && bs != "unset" { + if ok && bc != "" && bs != UnsetValue { blockComment = []byte(bc) } be, ok := def.Raw["block_comment_end"] - if !ok || be == "" || be == "unset" { + if !ok || be == "" || be == UnsetValue { return []error{fmt.Errorf("block_comment_end was expected, none were found")} } blockCommentEnd = []byte(be) @@ -49,7 +52,7 @@ func validate(r io.Reader, log logr.Logger, def *editorconfig.Definition) []erro maxLength := 0 tabWidth := def.TabWidth - if mll, ok := def.Raw["max_line_length"]; ok && mll != "off" && mll != "unset" { + if mll, ok := def.Raw["max_line_length"]; ok && mll != "off" && mll != UnsetValue { ml, err := strconv.Atoi(mll) if err != nil || ml < 0 { return []error{fmt.Errorf("max_line_length expected a non-negative number, got %s", mll)} @@ -99,7 +102,7 @@ func validate(r io.Reader, log logr.Logger, def *editorconfig.Definition) []erro } } - if def.IndentStyle != "" && def.IndentStyle != "unset" { + if def.IndentStyle != "" && def.IndentStyle != UnsetValue { if insideBlockComment && blockCommentEnd != nil { insideBlockComment = !isBlockCommentEnd(blockCommentEnd, data) } @@ -202,9 +205,9 @@ func overrideUsingPrefix(def *editorconfig.Definition, prefix string) error { } def.TabWidth = i case "trim_trailing_whitespace": - return fmt.Errorf("%v cannot be overriden yet, pr welcome", nk) + return fmt.Errorf("%v cannot be overridden yet, pr welcome", nk) case "insert_final_newline": - return fmt.Errorf("%v cannot be overriden yet, pr welcome", nk) + return fmt.Errorf("%v cannot be overridden yet, pr welcome", nk) } } } diff --git a/main.go b/main.go index 831b41b..f8fa381 100644 --- a/main.go +++ b/main.go @@ -1,129 +1,87 @@ package main import ( - "bytes" "flag" "fmt" "io" "os" - "path/filepath" - "runtime" - "strconv" "syscall" "golang.org/x/crypto/ssh/terminal" "github.com/editorconfig/editorconfig-core-go/v2" "github.com/go-logr/logr" - "github.com/logrusorgru/aurora" - "github.com/mattn/go-colorable" "k8s.io/klog/v2" "k8s.io/klog/v2/klogr" ) var ( version = "dev" - log logr.Logger ) -func walk(paths ...string) ([]string, error) { - files := make([]string, 0) - for _, path := range paths { - err := filepath.Walk(path, func(p string, i os.FileInfo, e error) error { - if e != nil { - return e - } - mode := i.Mode() - if mode.IsRegular() && !mode.IsDir() { - log.V(4).Info("index %s", p) - files = append(files, p) - } - return nil - }) - if err != nil { - return files, err - } - } - return files, nil +// option contains the environment of the program. +type option struct { + isTerminal bool + noColors bool + showAllErrors bool + summary bool + showErrorQuantity int + exclude string + log logr.Logger + stdout io.Writer } -// listFiles returns the list of files based on the input. -// -// When its empty, it relies on `git ls-files` first, which -// whould fail if `git` is not present or the current working -// directory is not managed by it. In that case, it work the -// current working directory. -// -// When args are given, it recursively walks into them. -func listFiles(args ...string) ([]string, error) { - if len(args) == 0 { - fs, err := gitLsFiles(".") - if err == nil { - return fs, nil - } - - log.Error(err, "git ls-files failure") - args = append(args, ".") +func main() { //nolint:funlen + flagVersion := false + opt := option{ + stdout: os.Stdout, + showErrorQuantity: 10, + log: klogr.New(), + isTerminal: terminal.IsTerminal(syscall.Stdout), } - return walk(args...) -} - -func main() { - var stdout io.Writer = os.Stdout - isTerminal := terminal.IsTerminal(syscall.Stdout) - if runtime.GOOS == "windows" { - stdout = colorable.NewColorableStdout() - } - - var flagVersion bool - - exclude := "" - noColors := false - summary := false - showAllErrors := false - showErrorQuantity := 10 - + // Flags klog.InitFlags(nil) flag.BoolVar(&flagVersion, "version", false, "print the version number") - flag.BoolVar(&noColors, "no_colors", false, "enable or disable colors") - flag.BoolVar(&summary, "summary", false, "enable the summary view") + flag.BoolVar(&opt.noColors, "no_colors", false, "enable or disable colors") + flag.BoolVar(&opt.summary, "summary", false, "enable the summary view") flag.BoolVar( - &showAllErrors, + &opt.showAllErrors, "show_all_errors", false, - fmt.Sprintf("display all errors for each file (otherwise %d are kept)", showErrorQuantity), + fmt.Sprintf("display all errors for each file (otherwise %d are kept)", opt.showErrorQuantity), ) - flag.StringVar(&exclude, "exclude", "", "paths to exclude") + flag.StringVar(&opt.exclude, "exclude", "", "paths to exclude") flag.Parse() if flagVersion { - fmt.Fprintf(stdout, "eclint %s\n", version) + fmt.Fprintf(opt.stdout, "eclint %s\n", version) return } - log = klogr.New() - args := flag.Args() - files, err := listFiles(args...) + files, err := listFiles(opt.log, args...) if err != nil { - log.Error(err, "error while handling the arguments") + opt.log.Error(err, "error while handling the arguments") flag.Usage() os.Exit(1) return } - au := aurora.NewAurora(isTerminal && !noColors) - log.V(1).Info("files", "count", len(files), "exclude", exclude) + opt.log.V(1).Info("files", "count", len(files), "exclude", opt.exclude) + + if opt.summary { + opt.showAllErrors = true + opt.showErrorQuantity = int(^uint(0) >> 1) + } c := 0 for _, filename := range files { // Skip excluded files - if exclude != "" { - ok, err := editorconfig.FnmatchCase(exclude, filename) + if opt.exclude != "" { + ok, err := editorconfig.FnmatchCase(opt.exclude, filename) if err != nil { - log.Error(err, "exclude pattern failure", "exclude", exclude) - fmt.Fprintf(stdout, "exclude pattern failure %s", err) + opt.log.Error(err, "exclude pattern failure", "exclude", opt.exclude) c++ break } @@ -132,89 +90,11 @@ func main() { } } - d := 0 - errs := lint(filename, log) - for _, err := range errs { - if err != nil { - if d == 0 && !summary { - fmt.Fprintf(stdout, "%s:\n", au.Magenta(filename)) - } - - if ve, ok := err.(validationError); ok { - log.V(4).Info("lint error", "error", ve) - if !summary { - vi := au.Green(strconv.Itoa(ve.index)) - vp := au.Green(strconv.Itoa(ve.position)) - fmt.Fprintf(stdout, "%s:%s: %s\n", vi, vp, ve.error) - l, err := errorAt(au, ve.line, ve.position-1) - if err != nil { - log.Error(err, "line formating failure", "error", ve) - continue - } - fmt.Fprintln(stdout, l) - } - } else { - log.V(4).Info("lint error", "filename", filename, "error", err) - fmt.Fprintln(stdout, err) - } - - if d >= showErrorQuantity && len(errs) > d { - fmt.Fprintln( - stdout, - fmt.Sprintf(" ... skipping at most %s errors", au.BrightRed(strconv.Itoa(len(errs)-d))), - ) - break - } - - d++ - c++ - } - } - if d > 0 { - if !summary { - fmt.Fprintln(stdout, "") - } else { - fmt.Fprintf(stdout, "%s: %d errors\n", au.Magenta(filename), d) - } - } + c += lintAndPrint(opt, filename) } + if c > 0 { - log.V(1).Info("Some errors were found.", "count", c) + opt.log.V(1).Info("Some errors were found.", "count", c) os.Exit(1) } } - -func errorAt(au aurora.Aurora, line []byte, position int) (string, error) { - b := bytes.NewBuffer(make([]byte, len(line))) - - if position > len(line) { - position = len(line) - } - - for i := 0; i < position; i++ { - if line[i] != cr && line[i] != lf { - if err := b.WriteByte(line[i]); err != nil { - return "", err - } - } - } - - // XXX this will break every non latin1 line. - s := " " - if position < len(line)-1 { - s = string(line[position : position+1]) - } - if _, err := b.WriteString(au.White(s).BgRed().String()); err != nil { - return "", err - } - - for i := position + 1; i < len(line); i++ { - if line[i] != cr && line[i] != lf { - if err := b.WriteByte(line[i]); err != nil { - return "", err - } - } - } - - return b.String(), nil -} diff --git a/print.go b/print.go new file mode 100644 index 0000000..f3fe624 --- /dev/null +++ b/print.go @@ -0,0 +1,105 @@ +package main + +import ( + "bytes" + "fmt" + "runtime" + "strconv" + + "github.com/logrusorgru/aurora" + "github.com/mattn/go-colorable" +) + +// lintAndPrint is the rich output of the program. +func lintAndPrint(opt option, filename string) int { + c := 0 + d := 0 + + stdout := opt.stdout + if runtime.GOOS == "windows" { + stdout = colorable.NewColorableStdout() + } + + au := aurora.NewAurora(opt.isTerminal && !opt.noColors) + errs := lint(filename, opt.log) + for _, err := range errs { + if err != nil { + if d == 0 && !opt.summary { + fmt.Fprintf(stdout, "%s:\n", au.Magenta(filename)) + } + + if ve, ok := err.(validationError); ok { + opt.log.V(4).Info("lint error", "error", ve) + if !opt.summary { + vi := au.Green(strconv.Itoa(ve.index)) + vp := au.Green(strconv.Itoa(ve.position)) + fmt.Fprintf(stdout, "%s:%s: %s\n", vi, vp, ve.error) + l, err := errorAt(au, ve.line, ve.position-1) + if err != nil { + opt.log.Error(err, "line formating failure", "error", ve) + continue + } + fmt.Fprintln(stdout, l) + } + } else { + opt.log.V(4).Info("lint error", "filename", filename, "error", err) + fmt.Fprintln(stdout, err) + } + + if d >= opt.showErrorQuantity && len(errs) > d { + fmt.Fprintln( + stdout, + fmt.Sprintf(" ... skipping at most %s errors", au.BrightRed(strconv.Itoa(len(errs)-d))), + ) + break + } + + d++ + c++ + } + } + if d > 0 { + if !opt.summary { + fmt.Fprintln(stdout, "") + } else { + fmt.Fprintf(stdout, "%s: %d errors\n", au.Magenta(filename), d) + } + } + return c +} + +// errorAt highlights the validationError position within the line. +func errorAt(au aurora.Aurora, line []byte, position int) (string, error) { + b := bytes.NewBuffer(make([]byte, len(line))) + + if position > len(line) { + position = len(line) + } + + for i := 0; i < position; i++ { + if line[i] != cr && line[i] != lf { + if err := b.WriteByte(line[i]); err != nil { + return "", err + } + } + } + + // XXX this will break every non latin1 line. + s := " " + if position < len(line)-1 { + s = string(line[position : position+1]) + } + if _, err := b.WriteString(au.White(s).BgRed().String()); err != nil { + return "", err + } + + for i := position + 1; i < len(line); i++ { + if line[i] != cr && line[i] != lf { + if err := b.WriteByte(line[i]); err != nil { + return "", err + } + } + } + + return b.String(), nil +} diff --git a/validators_test.go b/validators_test.go index 3bdfda3..c5652ef 100644 --- a/validators_test.go +++ b/validators_test.go @@ -126,7 +126,7 @@ func TestEndOfLine(t *testing.T) { } } -func TestEndOfLineFailures(t *testing.T) { +func TestEndOfLineFailures(t *testing.T) { //nolint:funlen tests := []struct { Name string EndOfLine string