Repository: go-logr/logr
Pull request #80 (2021-08-19 19:56:29)

Add more linters


Patch:
diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml
index 5c84b2d..c366cfc 100644
--- a/.github/workflows/lint.yaml
+++ b/.github/workflows/lint.yaml
@@ -15,14 +15,23 @@ jobs:
           docker run --rm -v `pwd`:/go/src/prj -w /go/src/prj \
             golangci/golangci-lint golangci-lint run \
                 -v \
+                --max-same-issues 10 \
                 --disable-all \
+                -E asciicheck \
                 -E deadcode \
+                -E errcheck \
+                -E forcetypeassert \
                 -E gocritic \
                 -E gofmt \
+                -E goimports \
+                -E gosimple \
                 -E govet \
                 -E ineffassign \
                 -E misspell \
                 -E revive \
                 -E staticcheck \
+                -E structcheck \
+                -E typecheck \
                 -E unused \
                 -E varcheck
+
diff --git a/funcr/funcr.go b/funcr/funcr.go
index 048ca58..69972bc 100644
--- a/funcr/funcr.go
+++ b/funcr/funcr.go
@@ -14,15 +14,9 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-// Package funcr implements formatting of structured log messages and
-// optionally captures the call site. This will not call String or
+// Package funcr implements github.com/go-logr/logr.Logger in terms of
+// an arbitrary "write" function.  This will not call String or
 // Error methods on values.
-//
-// The simplest way to use it is via its implementation of a
-// github.com/go-logr/logr.LogSink with output through an arbitrary
-// "write" function. Alternatively, funcr can also be embedded inside
-// a custom LogSink implementation. This is useful when the LogSink
-// needs to implement additional methods.
 package funcr
 
 import (
@@ -53,11 +47,20 @@ type Underlier interface {
 
 func newSink(fn func(prefix, args string), opts Options) logr.LogSink {
 	l := &fnlogger{
-		Formatter: NewFormatter(opts),
-		write:     fn,
+		prefix:       "",
+		values:       nil,
+		depth:        0,
+		write:        fn,
+		logCaller:    opts.LogCaller,
+		logTimestamp: opts.LogTimestamp,
+		verbosity:    opts.Verbosity,
+		helper:       opts.Helper,
+	}
+	if l.helper == nil {
+		// We have to have a valid function for GetCallStackHelper, so
+		// we might as well just cover the nil case once here.
+		l.helper = func() {}
 	}
-	// For skipping fnlogger.Info and fnlogger.Error.
-	l.Formatter.AddCallDepth(1)
 	return l
 }
 
@@ -74,6 +77,10 @@ type Options struct {
 	// Verbosity tells funcr which V logs to be write.  Higher values enable
 	// more logs.
 	Verbosity int
+
+	// Helper is an optional function that funcr will call to mark its own
+	// stack frames as helper functions.
+	Helper func()
 }
 
 // MessageClass indicates which category or categories of messages to consider.
@@ -88,46 +95,22 @@ const (
 
 const timestampFmt = "2006-01-02 15:04:05.000000"
 
-// fnlogger inherits some of its LogSink implementation from Formatter
-// and just needs to add some glue code.
 type fnlogger struct {
-	Formatter
-	write func(prefix, args string)
-}
-
-func (l fnlogger) WithName(name string) logr.LogSink {
-	l.Formatter.AddName(name)
-	return &l
-}
-
-func (l fnlogger) WithValues(kvList ...interface{}) logr.LogSink {
-	l.Formatter.AddValues(kvList)
-	return &l
-}
-
-func (l fnlogger) WithCallDepth(depth int) logr.LogSink {
-	l.Formatter.AddCallDepth(depth)
-	return &l
-}
-
-func (l fnlogger) Info(level int, msg string, kvList ...interface{}) {
-	prefix, args := l.FormatInfo(level, msg, kvList)
-	l.write(prefix, args)
-}
-
-func (l fnlogger) Error(err error, msg string, kvList ...interface{}) {
-	prefix, args := l.FormatError(err, msg, kvList)
-	l.write(prefix, args)
-}
-
-func (l fnlogger) GetUnderlying() func(prefix, args string) {
-	return l.write
+	prefix       string
+	values       []interface{}
+	depth        int
+	write        func(prefix, args string)
+	helper       func()
+	logCaller    MessageClass
+	logTimestamp bool
+	verbosity    int
 }
 
 // Assert conformance to the interfaces.
 var _ logr.LogSink = &fnlogger{}
 var _ logr.CallDepthLogSink = &fnlogger{}
 var _ Underlier = &fnlogger{}
+var _ logr.CallStackHelperLogSink = &fnlogger{}
 
 func flatten(kvList ...interface{}) string {
 	if len(kvList)%2 != 0 {
@@ -293,34 +276,9 @@ type callerID struct {
 	Line int    `json:"line"`
 }
 
-// NewFormatter constructs a Formatter.
-func NewFormatter(opts Options) Formatter {
-	f := Formatter{
-		prefix:       "",
-		values:       nil,
-		depth:        0,
-		logCaller:    opts.LogCaller,
-		logTimestamp: opts.LogTimestamp,
-		verbosity:    opts.Verbosity,
-	}
-	return f
-}
-
-// Formatter is an opaque struct which can be embedded in a LogSink
-// implementation. It should be constructed with NewFormatter. Some of
-// its methods directly implement logr.LogSink.
-type Formatter struct {
-	prefix       string
-	values       []interface{}
-	depth        int
-	logCaller    MessageClass
-	logTimestamp bool
-	verbosity    int
-}
-
-func (f Formatter) caller() callerID {
+func (l fnlogger) caller() callerID {
 	// +1 for this frame, +1 for Info/Error.
-	_, file, line, ok := runtime.Caller(f.depth + 2)
+	_, file, line, ok := runtime.Caller(l.depth + 2)
 	if !ok {
 		return callerID{"<unknown>", 0}
 	}
@@ -328,39 +286,37 @@ func (f Formatter) caller() callerID {
 }
 
 // Note that this receiver is a pointer, so depth can be saved.
-func (f *Formatter) Init(info logr.RuntimeInfo) {
-	f.depth += info.CallDepth
+func (l *fnlogger) Init(info logr.RuntimeInfo) {
+	l.depth += info.CallDepth
 }
 
-func (f Formatter) Enabled(level int) bool {
-	return level <= f.verbosity
+func (l fnlogger) Enabled(level int) bool {
+	return level <= l.verbosity
 }
 
-// FormatInfo flattens an Info log message into strings.
-// The prefix will be empty when no names were set.
-func (f Formatter) FormatInfo(level int, msg string, kvList []interface{}) (prefix, argsStr string) {
+func (l fnlogger) Info(level int, msg string, kvList ...interface{}) {
 	args := make([]interface{}, 0, 64) // using a constant here impacts perf
-	if f.logTimestamp {
+	if l.logTimestamp {
 		args = append(args, "ts", time.Now().Format(timestampFmt))
 	}
-	if f.logCaller == All || f.logCaller == Info {
-		args = append(args, "caller", f.caller())
+	if l.logCaller == All || l.logCaller == Info {
+		args = append(args, "caller", l.caller())
 	}
 	args = append(args, "level", level, "msg", msg)
-	args = append(args, f.values...)
+	args = append(args, l.values...)
 	args = append(args, kvList...)
-	return f.prefix, flatten(args...)
+	argsStr := flatten(args...)
+	l.helper()
+	l.write(l.prefix, argsStr)
 }
 
-// FormatInfo flattens an Error log message into strings.
-// The prefix will be empty when no names were set.
-func (f Formatter) FormatError(err error, msg string, kvList []interface{}) (prefix, argsStr string) {
+func (l fnlogger) Error(err error, msg string, kvList ...interface{}) {
 	args := make([]interface{}, 0, 64) // using a constant here impacts perf
-	if f.logTimestamp {
+	if l.logTimestamp {
 		args = append(args, "ts", time.Now().Format(timestampFmt))
 	}
-	if f.logCaller == All || f.logCaller == Error {
-		args = append(args, "caller", f.caller())
+	if l.logCaller == All || l.logCaller == Error {
+		args = append(args, "caller", l.caller())
 	}
 	args = append(args, "msg", msg)
 	var loggableErr interface{}
@@ -368,27 +324,40 @@ func (f Formatter) FormatError(err error, msg string, kvList []interface{}) (pre
 		loggableErr = err.Error()
 	}
 	args = append(args, "error", loggableErr)
-	args = append(args, f.values...)
+	args = append(args, l.values...)
 	args = append(args, kvList...)
-	return f.prefix, flatten(args...)
+	argsStr := flatten(args...)
+	l.helper()
+	l.write(l.prefix, argsStr)
 }
 
-// AddName appends the specified name.  funcr
+// WithName returns a new Logger with the specified name appended.  funcr
 // uses '/' characters to separate name elements.  Callers should not pass '/'
 // in the provided name string, but this library does not actually enforce that.
-func (f *Formatter) AddName(name string) {
-	if len(f.prefix) > 0 {
-		f.prefix += "/"
+func (l fnlogger) WithName(name string) logr.LogSink {
+	if len(l.prefix) > 0 {
+		l.prefix += "/"
 	}
-	f.prefix += name
+	l.prefix += name
+	return &l
 }
 
-func (f *Formatter) AddValues(kvList []interface{}) {
+func (l fnlogger) WithValues(kvList ...interface{}) logr.LogSink {
 	// Three slice args forces a copy.
-	n := len(f.values)
-	f.values = append(f.values[:n:n], kvList...)
+	n := len(l.values)
+	l.values = append(l.values[:n:n], kvList...)
+	return &l
+}
+
+func (l fnlogger) WithCallDepth(depth int) logr.LogSink {
+	l.depth += depth
+	return &l
+}
+
+func (l fnlogger) GetUnderlying() func(prefix, args string) {
+	return l.write
 }
 
-func (f *Formatter) AddCallDepth(depth int) {
-	f.depth += depth
+func (l fnlogger) GetCallStackHelper() func() {
+	return l.helper
 }


Test patch:
diff --git a/funcr/example_test.go b/funcr/example_test.go
index 7aa2e2d..6fae6a5 100644
--- a/funcr/example_test.go
+++ b/funcr/example_test.go
@@ -18,7 +18,6 @@ package funcr_test
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/go-logr/logr"
 	"github.com/go-logr/logr/funcr"
@@ -35,81 +34,3 @@ func ExampleUnderlier() {
 	}
 	// Output: hello world
 }
-
-// NewStdoutLogger returns a logr.Logger that prints to stdout.
-// It demonstrates how to implement a custom With* function which
-// controls whether INFO or ERROR are printed in front of the log
-// message.
-func NewStdoutLogger() logr.Logger {
-	l := &stdoutlogger{
-		Formatter: funcr.NewFormatter(funcr.Options{}),
-	}
-	return logr.New(l)
-}
-
-type stdoutlogger struct {
-	funcr.Formatter
-	logMsgType bool
-}
-
-func (l stdoutlogger) WithName(name string) logr.LogSink {
-	l.Formatter.AddName(name)
-	return &l
-}
-
-func (l stdoutlogger) WithValues(kvList ...interface{}) logr.LogSink {
-	l.Formatter.AddValues(kvList)
-	return &l
-}
-
-func (l stdoutlogger) WithCallDepth(depth int) logr.LogSink {
-	l.Formatter.AddCallDepth(depth)
-	return &l
-}
-
-func (l stdoutlogger) Info(level int, msg string, kvList ...interface{}) {
-	prefix, args := l.FormatInfo(level, msg, kvList)
-	l.write("INFO", prefix, args)
-}
-
-func (l stdoutlogger) Error(err error, msg string, kvList ...interface{}) {
-	prefix, args := l.FormatError(err, msg, kvList)
-	l.write("ERROR", prefix, args)
-}
-
-func (l stdoutlogger) write(msgType, prefix, args string) {
-	var parts []string
-	if l.logMsgType {
-		parts = append(parts, msgType)
-	}
-	if prefix != "" {
-		parts = append(parts, prefix)
-	}
-	parts = append(parts, args)
-	fmt.Println(strings.Join(parts, ": "))
-}
-
-// WithLogMsgType returns a copy of the logger with new settings for
-// logging the message type. It returns the original logger if the
-// underlying LogSink is not a stdoutlogger.
-func WithLogMsgType(log logr.Logger, logMsgType bool) logr.Logger {
-	if l, ok := log.GetSink().(*stdoutlogger); ok {
-		clone := *l
-		clone.logMsgType = logMsgType
-		log = log.WithSink(&clone)
-	}
-	return log
-}
-
-// Assert conformance to the interfaces.
-var _ logr.LogSink = &stdoutlogger{}
-var _ logr.CallDepthLogSink = &stdoutlogger{}
-
-func ExampleFormatter() {
-	l := NewStdoutLogger()
-	l.Info("no message type")
-	WithLogMsgType(l, true).Info("with message type")
-	// Output:
-	// "level"=0 "msg"="no message type"
-	// INFO: "level"=0 "msg"="with message type"
-}
diff --git a/funcr/funcr_test.go b/funcr/funcr_test.go
index 060b070..70d0d31 100644
--- a/funcr/funcr_test.go
+++ b/funcr/funcr_test.go
@@ -460,7 +460,7 @@ func TestInfoWithCallDepth(t *testing.T) {
 	t.Run("one", func(t *testing.T) {
 		cap := &capture{}
 		sink := newSink(cap.Func, Options{LogCaller: All})
-		dSink := sink.(logr.CallDepthLogSink)
+		dSink, _ := sink.(logr.CallDepthLogSink)
 		sink = dSink.WithCallDepth(1)
 		sink.Info(0, "msg")
 		_, file, line, _ := runtime.Caller(1)
@@ -475,7 +475,7 @@ func TestErrorWithCallDepth(t *testing.T) {
 	t.Run("one", func(t *testing.T) {
 		cap := &capture{}
 		sink := newSink(cap.Func, Options{LogCaller: All})
-		dSink := sink.(logr.CallDepthLogSink)
+		dSink, _ := sink.(logr.CallDepthLogSink)
 		sink = dSink.WithCallDepth(1)
 		sink.Error(fmt.Errorf("err"), "msg")
 		_, file, line, _ := runtime.Caller(1)
diff --git a/logr_test.go b/logr_test.go
index a8db00a..3f63818 100644
--- a/logr_test.go
+++ b/logr_test.go
@@ -279,7 +279,7 @@ func TestWithValues(t *testing.T) {
 	if calledWithValues != 1 {
 		t.Errorf("expected sink.WithValues() to be called once, got %d", calledWithValues)
 	}
-	if p := out.sink.(*testLogSink); p == sink {
+	if p, _ := out.sink.(*testLogSink); p == sink {
 		t.Errorf("expected output to be different from input, got in=%p, out=%p", sink, p)
 	}
 }
@@ -301,7 +301,7 @@ func TestWithName(t *testing.T) {
 	if calledWithName != 1 {
 		t.Errorf("expected sink.WithName() to be called once, got %d", calledWithName)
 	}
-	if p := out.sink.(*testLogSink); p == sink {
+	if p, _ := out.sink.(*testLogSink); p == sink {
 		t.Errorf("expected output to be different from input, got in=%p, out=%p", sink, p)
 	}
 }
@@ -313,7 +313,7 @@ func TestWithCallDepthNotImplemented(t *testing.T) {
 	logger := New(sink)
 
 	out := logger.WithCallDepth(depthInput)
-	if p := out.sink.(*testLogSink); p != sink {
+	if p, _ := out.sink.(*testLogSink); p != sink {
 		t.Errorf("expected output to be the same as input, got in=%p, out=%p", sink, p)
 	}
 }
@@ -335,7 +335,7 @@ func TestWithCallDepthImplemented(t *testing.T) {
 	if calledWithCallDepth != 1 {
 		t.Errorf("expected sink.WithCallDepth() to be called once, got %d", calledWithCallDepth)
 	}
-	p := out.sink.(*testCallDepthLogSink)
+	p, _ := out.sink.(*testCallDepthLogSink)
 	if p == sink {
 		t.Errorf("expected output to be different from input, got in=%p, out=%p", sink, p)
 	}
@@ -364,7 +364,7 @@ func TestWithCallDepthIncremental(t *testing.T) {
 	if calledWithCallDepth != depthInput {
 		t.Errorf("expected sink.WithCallDepth() to be called %d times, got %d", depthInput, calledWithCallDepth)
 	}
-	p := out.sink.(*testCallDepthLogSink)
+	p, _ := out.sink.(*testCallDepthLogSink)
 	if p == sink {
 		t.Errorf("expected output to be different from input, got in=%p, out=%p", sink, p)
 	}
@@ -392,11 +392,11 @@ func TestContext(t *testing.T) {
 	lctx := NewContext(ctx, logger)
 	if out, err := FromContext(lctx); err != nil {
 		t.Errorf("unexpected error: %v", err)
-	} else if p := out.sink.(*testLogSink); p != sink {
+	} else if p, _ := out.sink.(*testLogSink); p != sink {
 		t.Errorf("expected output to be the same as input, got in=%p, out=%p", sink, p)
 	}
 	out = FromContextOrDiscard(lctx)
-	if p := out.sink.(*testLogSink); p != sink {
+	if p, _ := out.sink.(*testLogSink); p != sink {
 		t.Errorf("expected output to be the same as input, got in=%p, out=%p", sink, p)
 	}
 }
diff --git a/testing/test.go b/testing/test.go
index 9978f8c..861357f 100644
--- a/testing/test.go
+++ b/testing/test.go
@@ -26,65 +26,9 @@ import (
 // NewTestLogger returns a logr.Logger that prints through a testing.T object.
 // Info logs are only enabled at V(0).
 func NewTestLogger(t *testing.T) logr.Logger {
-	l := &testlogger{
-		Formatter: funcr.NewFormatter(funcr.Options{}),
-		t:         t,
+	fn := func(prefix, args string) {
+		t.Helper()
+		t.Logf("%s: %s", prefix, args)
 	}
-	return logr.New(l)
+	return funcr.New(fn, funcr.Options{Helper: t.Helper})
 }
-
-// Underlier exposes access to the underlying testing.T instance. Since
-// callers only have a logr.Logger, they have to know which
-// implementation is in use, so this interface is less of an
-// abstraction and more of a way to test type conversion.
-type Underlier interface {
-	GetUnderlying() *testing.T
-}
-
-type testlogger struct {
-	funcr.Formatter
-	t *testing.T
-}
-
-func (l testlogger) WithName(name string) logr.LogSink {
-	l.Formatter.AddName(name)
-	return &l
-}
-
-func (l testlogger) WithValues(kvList ...interface{}) logr.LogSink {
-	l.Formatter.AddValues(kvList)
-	return &l
-}
-
-func (l testlogger) GetCallStackHelper() func() {
-	return l.t.Helper
-}
-
-func (l testlogger) Info(level int, msg string, kvList ...interface{}) {
-	prefix, args := l.FormatInfo(level, msg, kvList)
-	l.t.Helper()
-	if prefix != "" {
-		l.t.Logf("%s: %s", prefix, args)
-	} else {
-		l.t.Log(args)
-	}
-}
-
-func (l testlogger) Error(err error, msg string, kvList ...interface{}) {
-	prefix, args := l.FormatError(err, msg, kvList)
-	l.t.Helper()
-	if prefix != "" {
-		l.t.Logf("%s: %s", prefix, args)
-	} else {
-		l.t.Log(args)
-	}
-}
-
-func (l testlogger) GetUnderlying() *testing.T {
-	return l.t
-}
-
-// Assert conformance to the interfaces.
-var _ logr.LogSink = &testlogger{}
-var _ logr.CallStackHelperLogSink = &testlogger{}
-var _ Underlier = &testlogger{}
diff --git a/testing/test_test.go b/testing/test_test.go
index 6b69dad..e4678c3 100644
--- a/testing/test_test.go
+++ b/testing/test_test.go
@@ -23,13 +23,12 @@ import (
 	"github.com/go-logr/logr"
 )
 
-func TestLogger(t *testing.T) {
+func TestTestLogger(t *testing.T) {
 	log := NewTestLogger(t)
 	log.Info("info")
 	log.V(0).Info("V(0).info")
 	log.V(1).Info("v(1).info")
 	log.Error(fmt.Errorf("error"), "error")
-	log.WithName("testing").Info("with prefix")
 	Helper(log, "hello world")
 }