From 405120513f94d7fb68e8157bda64a445f4f9c0b3 Mon Sep 17 00:00:00 2001
From: Brian Picciano <mediocregopher@gmail.com>
Date: Thu, 24 Jan 2019 16:45:38 -0500
Subject: [PATCH] merr: move With/GetValue to kv.go, make nil error behavior be
 explicitly defined and tested

---
 merr/kv.go        | 30 ++++++++++++++++++++++++++++++
 merr/kv_test.go   |  7 +++++++
 merr/merr.go      | 28 ++++++++++------------------
 merr/merr_test.go |  1 +
 4 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/merr/kv.go b/merr/kv.go
index 91fd4c7..030b24a 100644
--- a/merr/kv.go
+++ b/merr/kv.go
@@ -5,6 +5,32 @@ import (
 	"path/filepath"
 )
 
+// WithValue returns a copy of the original error, automatically wrapping it if
+// the error is not from merr (see Wrap). The returned error has a value set on
+// with for the given key.
+//
+// visible determines whether or not the value is visible in the output of
+// Error.
+func WithValue(e error, k, v interface{}, visible bool) error {
+	if e == nil {
+		return nil
+	}
+	er := wrap(e, true, 1)
+	er.attr[k] = val{val: v, visible: visible}
+	return er
+}
+
+// GetValue returns the value embedded in the error for the given key, or nil if
+// the error isn't from this package or doesn't have that key embedded.
+func GetValue(e error, k interface{}) interface{} {
+	if e == nil {
+		return nil
+	}
+	return wrap(e, false, -1).attr[k].val
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
 // not really used for attributes, but w/e
 const attrKeyErr attrKey = "err"
 const attrKeyErrSrc attrKey = "errSrc"
@@ -27,6 +53,10 @@ func (kv KVer) KV() map[string]interface{} {
 // If any keys conflict then their type information will be included as part of
 // the key.
 func KV(e error) KVer {
+	if e == nil {
+		return KVer{}
+	}
+
 	er := wrap(e, false, 1)
 	kvm := make(map[string]interface{}, len(er.attr)+1)
 
diff --git a/merr/kv_test.go b/merr/kv_test.go
index 483b943..28a8b9b 100644
--- a/merr/kv_test.go
+++ b/merr/kv_test.go
@@ -8,6 +8,13 @@ import (
 )
 
 func TestKV(t *T) {
+	massert.Fatal(t, massert.All(
+		massert.Nil(WithValue(nil, "foo", "bar", true)),
+		massert.Nil(WithValue(nil, "foo", "bar", false)),
+		massert.Nil(GetValue(nil, "foo")),
+		massert.Len(KV(nil).KV(), 0),
+	))
+
 	er := New("foo")
 	kv := KV(er).KV()
 	massert.Fatal(t, massert.Comment(
diff --git a/merr/merr.go b/merr/merr.go
index e7eff4c..f162501 100644
--- a/merr/merr.go
+++ b/merr/merr.go
@@ -1,5 +1,11 @@
 // Package merr extends the errors package with features like key-value
 // attributes for errors, embedded stacktraces, and multi-errors.
+//
+// merr functions takes in generic errors of the built-in type. The returned
+// errors are wrapped by a type internal to merr, and appear to also be of the
+// generic error type. This means that equality checking will not work, unless
+// the Base function is used. If any functions are given nil they will also
+// return nil.
 package merr
 
 import (
@@ -39,6 +45,10 @@ type err struct {
 type attrKey string
 
 func wrap(e error, cp bool, skip int) *err {
+	if e == nil {
+		return nil
+	}
+
 	er, ok := e.(*err)
 	if !ok {
 		er := &err{err: e, attr: map[interface{}]val{}}
@@ -132,24 +142,6 @@ func (er *err) Error() string {
 	return sb.String()
 }
 
-// WithValue returns a copy of the original error, automatically wrapping it if
-// the error is not from merr (see Wrap). The returned error has a value set on
-// with for the given key.
-//
-// visible determines whether or not the value is visible in the output of
-// Error.
-func WithValue(e error, k, v interface{}, visible bool) error {
-	er := wrap(e, true, 1)
-	er.attr[k] = val{val: v, visible: visible}
-	return er
-}
-
-// GetValue returns the value embedded in the error for the given key, or nil if
-// the error isn't from this package or doesn't have that key embedded.
-func GetValue(e error, k interface{}) interface{} {
-	return wrap(e, false, -1).attr[k].val
-}
-
 // Base takes in an error and checks if it is merr's internal error type. If it
 // is then the underlying error which is being wrapped is returned. If it's not
 // then the passed in error is returned as-is.
diff --git a/merr/merr_test.go b/merr/merr_test.go
index bc765a1..6077004 100644
--- a/merr/merr_test.go
+++ b/merr/merr_test.go
@@ -31,6 +31,7 @@ func TestBase(t *T) {
 	errFoo, errBar := errors.New("foo"), errors.New("bar")
 	erFoo := Wrap(errFoo)
 	massert.Fatal(t, massert.All(
+		massert.Nil(Base(nil)),
 		massert.Equal(errFoo, Base(erFoo)),
 		massert.Equal(errBar, Base(errBar)),
 		massert.Not(massert.Equal(errFoo, erFoo)),