From bd55b2795bd2887906db513e50d32d4437ea3809 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Fri, 29 Nov 2024 11:08:19 +0200 Subject: [PATCH] tools/linters: ignore log lines This commit adds a feature to our custom lll linter which will ignore log lines (both single and multi-lined log lines) for the lll linter. --- tools/linters/ll.go | 35 +++++++++++++++--- tools/linters/ll_test.go | 78 ++++++++++++++++++++++++++++++++++------ 2 files changed, 97 insertions(+), 16 deletions(-) diff --git a/tools/linters/ll.go b/tools/linters/ll.go index 2aff7e19b..7170447de 100644 --- a/tools/linters/ll.go +++ b/tools/linters/ll.go @@ -12,6 +12,7 @@ import ( "go/token" "os" "path/filepath" + "regexp" "strings" "unicode/utf8" @@ -25,12 +26,14 @@ const ( defaultMaxLineLen = 80 defaultTabWidthInSpaces = 8 + defaultLogRegex = `^\s*.*(L|l)og\.` ) // LLConfig is the configuration for the ll linter. type LLConfig struct { - LineLength int `json:"line-length"` - TabWidth int `json:"tab-width"` + LineLength int `json:"line-length"` + TabWidth int `json:"tab-width"` + LogRegex string `json:"log-regex"` } // New creates a new LLPlugin from the given settings. It satisfies the @@ -48,6 +51,9 @@ func New(settings any) (register.LinterPlugin, error) { if cfg.TabWidth == 0 { cfg.TabWidth = defaultTabWidthInSpaces } + if cfg.LogRegex == "" { + cfg.LogRegex = defaultLogRegex + } return &LLPlugin{cfg: cfg}, nil } @@ -79,13 +85,16 @@ func (l *LLPlugin) GetLoadMode() string { } func (l *LLPlugin) run(pass *analysis.Pass) (any, error) { - var spaces = strings.Repeat(" ", l.cfg.TabWidth) + var ( + spaces = strings.Repeat(" ", l.cfg.TabWidth) + logRegex = regexp.MustCompile(l.cfg.LogRegex) + ) for _, f := range pass.Files { fileName := getFileName(pass, f) issues, err := getLLLIssuesForFile( - fileName, l.cfg.LineLength, spaces, + fileName, l.cfg.LineLength, spaces, logRegex, ) if err != nil { return nil, err @@ -114,7 +123,7 @@ type issue struct { } func getLLLIssuesForFile(filename string, maxLineLen int, - tabSpaces string) ([]*issue, error) { + tabSpaces string, logRegex *regexp.Regexp) ([]*issue, error) { f, err := os.Open(filename) if err != nil { @@ -126,6 +135,7 @@ func getLLLIssuesForFile(filename string, maxLineLen int, res []*issue lineNumber int multiImportEnabled bool + multiLinedLog bool ) // Scan over each line. @@ -167,6 +177,21 @@ func getLLLIssuesForFile(filename string, maxLineLen int, continue } + // Check if the line matches the log pattern. + if logRegex.MatchString(line) { + multiLinedLog = !strings.HasSuffix(line, ")") + continue + } + + if multiLinedLog { + // Check for the end of a multiline log call. + if strings.HasSuffix(line, ")") { + multiLinedLog = false + } + + continue + } + // Otherwise, we can check the length of the line and report if // it exceeds the maximum line length. lineLen := utf8.RuneCountInString(line) diff --git a/tools/linters/ll_test.go b/tools/linters/ll_test.go index 9c3b7b44a..1ab4ae352 100644 --- a/tools/linters/ll_test.go +++ b/tools/linters/ll_test.go @@ -2,6 +2,7 @@ package linters import ( "os" + "regexp" "strings" "testing" @@ -16,12 +17,14 @@ func TestGetLLLIssuesForFile(t *testing.T) { testCases := []struct { name string content string + logRegex string expectedIssue []string }{ { name: "Single long line", content: ` fmt.Println("This is a very long line that exceeds the maximum length and should be flagged by the linter.")`, + logRegex: defaultLogRegex, expectedIssue: []string{ "the line is 140 characters long, which " + "exceeds the maximum of 80 characters.", @@ -32,6 +35,7 @@ func TestGetLLLIssuesForFile(t *testing.T) { content: ` fmt.Println("This is a very long line that exceeds the maximum length and should be flagged by the linter.") fmt.Println("This is a another very long line that exceeds the maximum length and should be flagged by the linter.")`, + logRegex: defaultLogRegex, expectedIssue: []string{ "the line is 140 characters long, which " + "exceeds the maximum of 80 characters.", @@ -40,20 +44,24 @@ func TestGetLLLIssuesForFile(t *testing.T) { }, }, { - name: "Short lines", + name: "Short lines", + logRegex: defaultLogRegex, content: ` fmt.Println("Short line")`, }, { - name: "Directive ignored", - content: `//go:generate something very very very very very very very very very long and complex here wowowow`, + name: "Directive ignored", + logRegex: defaultLogRegex, + content: `//go:generate something very very very very very very very very very long and complex here wowowow`, }, { - name: "Long single line import", - content: `import "github.com/lightningnetwork/lnd/lnrpc/walletrpc/more/more/more/more/more/more/ok/that/is/enough"`, + name: "Long single line import", + logRegex: defaultLogRegex, + content: `import "github.com/lightningnetwork/lnd/lnrpc/walletrpc/more/more/more/more/more/more/ok/that/is/enough"`, }, { - name: "Multi-line import", + name: "Multi-line import", + logRegex: defaultLogRegex, content: ` import ( "os" @@ -62,20 +70,68 @@ func TestGetLLLIssuesForFile(t *testing.T) { )`, }, { - name: "Long single line log", + name: "Long single line log", + logRegex: defaultLogRegex, content: ` - log.Infof("This is a very long log line but since it is a log line, it should be skipped by the linter.")`, + log.Infof("This is a very long log line but since it is a log line, it should be skipped by the linter."), + rpcLog.Info("Another long log line with a slightly different name and should still be skipped")`, + }, + { + name: "Long single line log followed by a non-log line", + logRegex: defaultLogRegex, + content: ` + log.Infof("This is a very long log line but since it is a log line, it should be skipped by the linter.") + fmt.Println("This is a very long line that exceeds the maximum length and should be flagged by the linter.")`, expectedIssue: []string{ - "the line is 121 characters long, which " + + "the line is 140 characters long, which " + + "exceeds the maximum of 80 characters.", + }, + }, + { + name: "Multi-line log", + logRegex: defaultLogRegex, + content: ` + log.Infof("This is a very long log line but + since it is a log line, it + should be skipped by the linter.")`, + }, + { + name: "Multi-line log followed by a non-log line", + logRegex: defaultLogRegex, + content: ` + log.Infof("This is a very long log line but + since it is a log line, it + should be skipped by the linter.") + fmt.Println("This is a very long line that + exceeds the maximum length and + should be flagged by the linter.")`, + expectedIssue: []string{ + "the line is 82 characters long, which " + + "exceeds the maximum of 80 characters.", + }, + }, + { + name: "Only skip 'S' logs", + logRegex: `^\s*.*(L|l)og\.(Info|Debug|Trace|Warn|Error|Critical)S\(`, + content: ` + log.Infof("A long log line but it is not an S log and so should be caught") + log.InfoS("This is a very long log line but + since it is an 'S' log line, it + should be skipped by the linter.") + log.TraceS("Another S log that should be skipped by the linter")`, + expectedIssue: []string{ + "the line is 107 characters long, which " + "exceeds the maximum of 80 characters.", }, }, } - tabSpaces := strings.Repeat(" ", 8) + tabSpaces := strings.Repeat(" ", defaultTabWidthInSpaces) for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + logRegex := regexp.MustCompile(tc.logRegex) + // Write content to a temporary file. tmpFile := t.TempDir() + "/test.go" err := os.WriteFile(tmpFile, []byte(tc.content), 0644) @@ -83,7 +139,7 @@ func TestGetLLLIssuesForFile(t *testing.T) { // Run the linter on the file. issues, err := getLLLIssuesForFile( - tmpFile, 80, tabSpaces, + tmpFile, defaultMaxLineLen, tabSpaces, logRegex, ) require.NoError(t, err)