Repository: go-logr/logr (go)

Issue:
Should funcr use fmt.Stringer() if possible?

Pull request:
funcr: support fmt.Stringer
It seems like a logical thing to do now, though I still feel like I could argue that call-sites can CHOOSE to call `String()`, which saves epsilon overhead on every call...

Fixes #95 


Patch:
diff --git a/funcr/funcr.go b/funcr/funcr.go
index 4383f60..8af7ba2 100644
--- a/funcr/funcr.go
+++ b/funcr/funcr.go
@@ -15,14 +15,16 @@ limitations under the License.
 */
 
 // Package funcr implements formatting of structured log messages and
-// optionally captures the call site. This will not call String or
-// Error methods on values.
+// optionally captures the call site.
 //
 // 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.
+//
+// This will respect logr.Marshaler, fmt.Stringer, and error interfaces for
+// values which are being logged.
 package funcr
 
 import (
@@ -168,12 +170,23 @@ const (
 
 // TODO: This is not fast. Most of the overhead goes here.
 func prettyWithFlags(value interface{}, flags uint32) string {
-	// Handling the most common types without reflect is a small perf win.
-	switch v := value.(type) {
-	case logr.Marshaler:
-		// Replace the value with what the value wants to get logged.
+	// Handle types that take full control of logging.
+	if v, ok := value.(logr.Marshaler); ok {
+		// Replace the value with what the type wants to get logged.
 		// That then gets handled below via reflection.
 		value = v.MarshalLog()
+	}
+
+	// Handle types that want to format themselves.
+	switch v := value.(type) {
+	case fmt.Stringer:
+		value = v.String()
+	case error:
+		value = v.Error()
+	}
+
+	// Handling the most common types without reflect is a small perf win.
+	switch v := value.(type) {
 	case bool:
 		return strconv.FormatBool(v)
 	case string:


Test patch:
diff --git a/funcr/funcr_test.go b/funcr/funcr_test.go
index 1c201c9..fe02a83 100644
--- a/funcr/funcr_test.go
+++ b/funcr/funcr_test.go
@@ -35,6 +35,32 @@ func ptrstr(s string) *string {
 	return &s
 }
 
+// Logging this should result in the MarshalLog() value.
+type Tmarshaler string
+
+func (t Tmarshaler) MarshalLog() interface{} {
+	return struct{ Inner string }{"I am a logr.Marshaler"}
+}
+
+func (t Tmarshaler) String() string {
+	return "String(): you should not see this"
+}
+
+func (t Tmarshaler) Error() string {
+	return "Error(): you should not see this"
+}
+
+// Logging this should result in the String() value.
+type Tstringer string
+
+func (t Tstringer) String() string {
+	return "I am a fmt.Stringer"
+}
+
+func (t Tstringer) Error() string {
+	return "Error(): you should not see this"
+}
+
 func TestPretty(t *testing.T) {
 	cases := []struct {
 		val interface{}
@@ -92,7 +118,6 @@ func TestPretty(t *testing.T) {
 				"nine": 3,
 			},
 		},
-		{val: fmt.Errorf("error")},
 		{
 			val: struct {
 				X int `json:"x"`
@@ -129,6 +154,18 @@ func TestPretty(t *testing.T) {
 				D: interface{}(2),
 			},
 		},
+		{
+			val: Tmarshaler("foobar"),
+			exp: `{"Inner":"I am a logr.Marshaler"}`,
+		},
+		{
+			val: Tstringer("foobar"),
+			exp: `"I am a fmt.Stringer"`,
+		},
+		{
+			val: fmt.Errorf("error"),
+			exp: `"error"`,
+		},
 	}
 
 	for i, tc := range cases {