ronantakizawa/codereview-bench huggingface dataset:
Repository: uber-go/zap
File: global.go (Go)

Code before:
//
// It returns a function to restore the original prefix and flags and reset the
// standard library's output to os.Stdout.
func RedirectStdLogAt(l *Logger, level zapcore.Level) (func(), error) {
	return redirectStdLogAt(l, level)
}

func redirectStdLogAt(l *Logger, level zapcore.Level) (func(), error) {
	flags := log.Flags()
	prefix := log.Prefix()
	log.SetFlags(0)
	log.SetPrefix("")
	logger := l.WithOptions(AddCallerSkip(_stdLogDefaultDepth + _loggerWriterDepth))
	logFunc, err := levelToFunc(logger, level)
	if err != nil {
		return nil, err
	}
	log.SetOutput(&loggerWriter{logFunc})
	return func() {
		log.SetFlags(flags)
		log.SetPrefix(prefix)
		log.SetOutput(os.Stderr)
	}, nil
}

func levelToFunc(logger *Logger, lvl zapcore.Level) (f func(string, ...zapcore.Field), err error) {
	switch lvl {
	case DebugLevel:
		f = logger.Debug
	case InfoLevel:
		f = logger.Info
	case WarnLevel:
		f = logger.Warn
	case ErrorLevel:
		f = logger.Error
	case DPanicLevel:
		f = logger.DPanic
	case PanicLevel:
		f = logger.Panic
	case FatalLevel:
		f = logger.Fatal
	default:
		err = fmt.Errorf("unrecognized level: %q", lvl)
	}
	return
}

type loggerWriter struct {
	logFunc func(msg string, fields ...zapcore.Field)
}


Review comment (suggestion):
This felt like a place where named return values simplified the code; happy to remove them if any reviewers disagree.

Code after:
// handles caller annotations, timestamps, etc., it automatically disables the
// standard library's annotations and prefixing.
//
// It returns a function to restore the original prefix and flags and reset the
// standard library's output to os.Stdout.
func RedirectStdLogAt(l *Logger, level zapcore.Level) (func(), error) {
	return redirectStdLogAt(l, level)
}

func redirectStdLogAt(l *Logger, level zapcore.Level) (func(), error) {
	flags := log.Flags()
	prefix := log.Prefix()
	log.SetFlags(0)
	log.SetPrefix("")
	logger := l.WithOptions(AddCallerSkip(_stdLogDefaultDepth + _loggerWriterDepth))
	logFunc, err := levelToFunc(logger, level)
	if err != nil {
		return nil, err
	}
	log.SetOutput(&loggerWriter{logFunc})
	return func() {
		log.SetFlags(flags)
		log.SetPrefix(prefix)
		log.SetOutput(os.Stderr)
	}, nil
}

func levelToFunc(logger *Logger, lvl zapcore.Level) (func(string, ...zapcore.Field), error) {
	switch lvl {
	case DebugLevel:
		return logger.Debug, nil
	case InfoLevel:
		return logger.Info, nil
	case WarnLevel:
		return logger.Warn, nil
	case ErrorLevel:
		return logger.Error, nil
	case DPanicLevel:
		return logger.DPanic, nil
	case PanicLevel:
		return logger.Panic, nil
	case FatalLevel:
		return logger.Fatal, nil
	}
	return nil, fmt.Errorf("unrecognized level: %q", lvl)
}

type loggerWriter struct {
	logFunc func(msg string, fields ...zapcore.Field)
}
Repository: uber-go/zap
File: zaptest/logger_test.go (Go)

Code before:
	t.Fail()
	t.TB.FailNow()
}

func (t *testLogSpy) Logf(format string, args ...interface{}) {
	// Log messages are in the format,
	//
	//   2017-10-27T13:03:01.000-0700	DEBUG	your message here	{data here}
	//
	// We strip the first part of these messages because we can't really test
	// for the timestamp from these tests.
	m := fmt.Sprintf(format, args...)
	m = m[strings.IndexByte(m, '\t')+1:]

	t.mu.Lock()
	t.Messages = append(t.Messages, m)
	t.mu.Unlock()

	t.TB.Log(m)
}

func (t *testLogSpy) AssertMessages(msgs ...string) {
	assert.Equal(t.TB, msgs, t.Messages, "logged messages did not match")
}

func (t *testLogSpy) AssertFailed(v bool) {
	assert.Equal(t.TB, v, t.failed, "test status did not match")
}


Review comment (bug):
Can we make this an unexported method and have methods `AssertFailed()` and `AssertPassed()` rather than a boolean argument?

Code after:
	t.mu.Unlock()
}

func (t *testLogSpy) Failed() bool {
	t.mu.RLock()
	failed := t.failed
	t.mu.RUnlock()
	return failed
}

func (t *testLogSpy) FailNow() {
	t.Fail()
	t.TB.FailNow()
}

func (t *testLogSpy) Logf(format string, args ...interface{}) {
	// Log messages are in the format,
	//
	//   2017-10-27T13:03:01.000-0700	DEBUG	your message here	{data here}
	//
	// We strip the first part of these messages because we can't really test
	// for the timestamp from these tests.
	m := fmt.Sprintf(format, args...)
	m = m[strings.IndexByte(m, '\t')+1:]

	t.mu.Lock()
	t.Messages = append(t.Messages, m)
	t.mu.Unlock()

	t.TB.Log(m)
}

func (t *testLogSpy) AssertMessages(msgs ...string) {
	assert.Equal(t.TB, msgs, t.Messages, "logged messages did not match")
}

func (t *testLogSpy) AssertPassed() {
	t.assertFailed(false, "expected test to pass")
}

func (t *testLogSpy) AssertFailed() {
	t.assertFailed(true, "expected test to fail")
}

func (t *testLogSpy) assertFailed(v bool, msg string) {
	assert.Equal(t.TB, v, t.failed, msg)
}
Repository: uber-go/zap
File: zaptest/logger_test.go (Go)

Code before:
	}))

	log.Info("foo") // this fails

	ts.AssertFailed(true)
	if assert.Len(t, ts.Messages, 1, "expected a log message") {
		assert.Regexp(t, `write error: failed`, ts.Messages[0])
	}
}

// testLogSpy is a testing.TB that captures logged messages.
type testLogSpy struct {
	testing.TB

	mu       sync.RWMutex
	failed   bool
	Messages []string
}

func newTestLogSpy(t testing.TB) *testLogSpy {
	return &testLogSpy{TB: t}
}

func (t *testLogSpy) Fail() {
	t.mu.Lock()
	t.failed = true
	t.mu.Unlock()
}

func (t *testLogSpy) Failed() bool {
	t.mu.RLock()
	failed := t.failed
	t.mu.RUnlock()
	return failed
}

func (t *testLogSpy) FailNow() {
	t.Fail()
	t.TB.FailNow()
}

func (t *testLogSpy) Logf(format string, args ...interface{}) {
	// Log messages are in the format,
	//
	//   2017-10-27T13:03:01.000-0700	DEBUG	your message here	{data here}
	//
	// We strip the first part of these messages because we can't really test
	// for the timestamp from these tests.
	m := fmt.Sprintf(format, args...)
	m = m[strings.IndexByte(m, '\t')+1:]


Review comment (suggestion):
optional/can follow up:
this spy is only used internally, and I don't think any of the tests are concurrent, so we might be able to just remove the mutex.


Code after:
	ts := newTestLogSpy(t)
	defer ts.AssertFailed()

	log := NewLogger(ts)

	// Replace with a core that fails.
	log = log.WithOptions(zap.WrapCore(func(zapcore.Core) zapcore.Core {
		return zapcore.NewCore(
			zapcore.NewConsoleEncoder(zap.NewDevelopmentEncoderConfig()),
			zapcore.Lock(zapcore.AddSync(ztest.FailWriter{})),
			zapcore.DebugLevel,
		)
	}))

	log.Info("foo") // this fails

	if assert.Len(t, ts.Messages, 1, "expected a log message") {
		assert.Regexp(t, `write error: failed`, ts.Messages[0])
	}
}

// testLogSpy is a testing.TB that captures logged messages.
type testLogSpy struct {
	testing.TB

	mu       sync.RWMutex
	failed   bool
	Messages []string
}

func newTestLogSpy(t testing.TB) *testLogSpy {
	return &testLogSpy{TB: t}
}

func (t *testLogSpy) Fail() {
	t.mu.Lock()
	t.failed = true
	t.mu.Unlock()
}

func (t *testLogSpy) Failed() bool {
	t.mu.RLock()
	failed := t.failed
	t.mu.RUnlock()
	return failed
}

func (t *testLogSpy) FailNow() {
	t.Fail()
	t.TB.FailNow()
}
Repository: uber-go/zap
File: sink.go (Go)

Code before:
		return errors.New("can't register a sink factory for empty string")
	}
	normalized, err := normalizeScheme(scheme)
	if err != nil {
		return fmt.Errorf("%q is not a valid scheme: %v", scheme, err)
	}
	if _, ok := _sinkFactories[normalized]; ok {
		return fmt.Errorf("sink factory already registered for scheme %q", normalized)
	}
	_sinkFactories[normalized] = factory
	return nil
}

func newSink(rawURL string) (Sink, error) {
	u, err := url.Parse(rawURL)
	if err != nil {
		return nil, fmt.Errorf("can't parse %q as a URL: %v", rawURL, err)
	}
	if u.Scheme == "" {
		u.Scheme = schemeFile
	}

	_sinkMutex.RLock()
	defer _sinkMutex.RUnlock()

	factory, ok := _sinkFactories[u.Scheme]
	if !ok {
		return nil, &errSinkNotFound{u.Scheme}
	}
	return factory(u)
}

func newFileSink(u *url.URL) (Sink, error) {
	switch u.Path {
	case "stdout":
		return nopCloserSink{os.Stdout}, nil
	case "stderr":
		return nopCloserSink{os.Stderr}, nil
	}
	return os.OpenFile(u.Path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
}

func normalizeScheme(s string) (string, error) {
	// https://tools.ietf.org/html/rfc3986#section-3.1
	s = strings.ToLower(s)
	if first := s[0]; 'a' > first || 'z' < first {
		return "", errors.New("must start with a letter")
	}
	if len(s) < 2 {
		return s, nil
	}

Review comment (suggestion):
optionally: can we hold the `RLock` just while we're looking up the factory?
```go

_sinkMutex.RLock()
factory, ok := _sinkFactories[u.Scheme]
_sinkMutex.RUnlock()
```

Shouldn't be an issue since it's just an `RLock`, but want to avoid holding on to locks while triggering the factory which is user-supplied.

Code after:
		return errors.New("can't register a sink factory for empty string")
	}
	normalized, err := normalizeScheme(scheme)
	if err != nil {
		return fmt.Errorf("%q is not a valid scheme: %v", scheme, err)
	}
	if _, ok := _sinkFactories[normalized]; ok {
		return fmt.Errorf("sink factory already registered for scheme %q", normalized)
	}
	_sinkFactories[normalized] = factory
	return nil
}

func newSink(rawURL string) (Sink, error) {
	u, err := url.Parse(rawURL)
	if err != nil {
		return nil, fmt.Errorf("can't parse %q as a URL: %v", rawURL, err)
	}
	if u.Scheme == "" {
		u.Scheme = schemeFile
	}

	_sinkMutex.RLock()
	factory, ok := _sinkFactories[u.Scheme]
	_sinkMutex.RUnlock()
	if !ok {
		return nil, &errSinkNotFound{u.Scheme}
	}
	return factory(u)
}

func newFileSink(u *url.URL) (Sink, error) {
	if u.User != nil {
		return nil, fmt.Errorf("user and password not allowed with file URLs: got %v", u)
	}
	if u.Fragment != "" {
		return nil, fmt.Errorf("fragments not allowed with file URLs: got %v", u)
	}
	if u.RawQuery != "" {
		return nil, fmt.Errorf("query parameters not allowed with file URLs: got %v", u)
	}
	// Error messages are better if we check hostname and port separately.
	if u.Port() != "" {
		return nil, fmt.Errorf("ports not allowed with file URLs: got %v", u)
	}
	if hn := u.Hostname(); hn != "" && hn != "localhost" {
		return nil, fmt.Errorf("file URLs must leave host empty or use localhost: got %v", u)
	}
	switch u.Path {
	case "stdout":
		return nopCloserSink{os.Stdout}, nil
Repository: uber-go/zap
File: sink.go (Go)

Code before:
	if _, ok := _sinkFactories[normalized]; ok {
		return fmt.Errorf("sink factory already registered for scheme %q", normalized)
	}
	_sinkFactories[normalized] = factory
	return nil
}

func newSink(rawURL string) (Sink, error) {
	u, err := url.Parse(rawURL)
	if err != nil {
		return nil, fmt.Errorf("can't parse %q as a URL: %v", rawURL, err)
	}
	if u.Scheme == "" {
		u.Scheme = schemeFile
	}

	_sinkMutex.RLock()
	defer _sinkMutex.RUnlock()

	factory, ok := _sinkFactories[u.Scheme]
	if !ok {
		return nil, &errSinkNotFound{u.Scheme}
	}
	return factory(u)
}

func newFileSink(u *url.URL) (Sink, error) {
	switch u.Path {
	case "stdout":
		return nopCloserSink{os.Stdout}, nil
	case "stderr":
		return nopCloserSink{os.Stderr}, nil
	}
	return os.OpenFile(u.Path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
}

func normalizeScheme(s string) (string, error) {
	// https://tools.ietf.org/html/rfc3986#section-3.1
	s = strings.ToLower(s)
	if first := s[0]; 'a' > first || 'z' < first {
		return "", errors.New("must start with a letter")
	}
	if len(s) < 2 {
		return s, nil
	}
	for i := 1; i < len(s); i++ { // iterate over bytes, not runes
		c := s[i]
		switch {
		case 'a' <= c && c <= 'z':
			continue
		case '0' <= c && c <= '9':

Review comment (suggestion):
optional: should we ensure there's no fragments etc? ignoring seems OK too, but it might be a little surprising

Code after:
	if _, ok := _sinkFactories[normalized]; ok {
		return fmt.Errorf("sink factory already registered for scheme %q", normalized)
	}
	_sinkFactories[normalized] = factory
	return nil
}

func newSink(rawURL string) (Sink, error) {
	u, err := url.Parse(rawURL)
	if err != nil {
		return nil, fmt.Errorf("can't parse %q as a URL: %v", rawURL, err)
	}
	if u.Scheme == "" {
		u.Scheme = schemeFile
	}

	_sinkMutex.RLock()
	factory, ok := _sinkFactories[u.Scheme]
	_sinkMutex.RUnlock()
	if !ok {
		return nil, &errSinkNotFound{u.Scheme}
	}
	return factory(u)
}

func newFileSink(u *url.URL) (Sink, error) {
	if u.User != nil {
		return nil, fmt.Errorf("user and password not allowed with file URLs: got %v", u)
	}
	if u.Fragment != "" {
		return nil, fmt.Errorf("fragments not allowed with file URLs: got %v", u)
	}
	if u.RawQuery != "" {
		return nil, fmt.Errorf("query parameters not allowed with file URLs: got %v", u)
	}
	// Error messages are better if we check hostname and port separately.
	if u.Port() != "" {
		return nil, fmt.Errorf("ports not allowed with file URLs: got %v", u)
	}
	if hn := u.Hostname(); hn != "" && hn != "localhost" {
		return nil, fmt.Errorf("file URLs must leave host empty or use localhost: got %v", u)
	}
	switch u.Path {
	case "stdout":
		return nopCloserSink{os.Stdout}, nil
	case "stderr":
		return nopCloserSink{os.Stderr}, nil
	}
	return os.OpenFile(u.Path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
}
Repository: uber-go/zap
File: zapcore/field.go (Go)

Code before:
		enc.AddInt8(f.Key, int8(f.Integer))
	case StringType:
		enc.AddString(f.Key, f.String)
	case TimeType:
		if f.Interface != nil {
			enc.AddTime(f.Key, time.Unix(0, f.Integer).In(f.Interface.(*time.Location)))
		} else {
			// Fall back to UTC if location is nil.
			enc.AddTime(f.Key, time.Unix(0, f.Integer))
		}
	case Uint64Type:
		enc.AddUint64(f.Key, uint64(f.Integer))
	case Uint32Type:
		enc.AddUint32(f.Key, uint32(f.Integer))
	case Uint16Type:
		enc.AddUint16(f.Key, uint16(f.Integer))
	case Uint8Type:
		enc.AddUint8(f.Key, uint8(f.Integer))
	case UintptrType:
		enc.AddUintptr(f.Key, uintptr(f.Integer))
	case ReflectType:
		err = enc.AddReflected(f.Key, f.Interface)
	case NamespaceType:
		enc.OpenNamespace(f.Key)
	case StringerType:
		defer func() {
			if r := recover(); r != nil {
				var msg string

				switch v := r.(type) {
				case error:
					msg = v.Error()
				case string:
					msg = v
				default:
					msg = "<unknown>"
				}

				enc.AddString(fmt.Sprintf("%sError", f.Key), msg)
			}
		}()

		enc.AddString(f.Key, f.Interface.(fmt.Stringer).String())
	case ErrorType:
		encodeError(f.Key, f.Interface.(error), enc)
	case SkipType:
		break
	default:
		panic(fmt.Sprintf("unknown field type: %v", f))
	}


Review comment (suggestion):
Given the function is pretty large and this is a large block, it'd be better to move the logic into a separate `encodeStringer` function

Code after:
		enc.AddInt8(f.Key, int8(f.Integer))
	case StringType:
		enc.AddString(f.Key, f.String)
	case TimeType:
		if f.Interface != nil {
			enc.AddTime(f.Key, time.Unix(0, f.Integer).In(f.Interface.(*time.Location)))
		} else {
			// Fall back to UTC if location is nil.
			enc.AddTime(f.Key, time.Unix(0, f.Integer))
		}
	case Uint64Type:
		enc.AddUint64(f.Key, uint64(f.Integer))
	case Uint32Type:
		enc.AddUint32(f.Key, uint32(f.Integer))
	case Uint16Type:
		enc.AddUint16(f.Key, uint16(f.Integer))
	case Uint8Type:
		enc.AddUint8(f.Key, uint8(f.Integer))
	case UintptrType:
		enc.AddUintptr(f.Key, uintptr(f.Integer))
	case ReflectType:
		err = enc.AddReflected(f.Key, f.Interface)
	case NamespaceType:
		enc.OpenNamespace(f.Key)
	case StringerType:
		err = encodeStringer(f.Key, f.Interface, enc)
	case ErrorType:
		encodeError(f.Key, f.Interface.(error), enc)
	case SkipType:
		break
	default:
		panic(fmt.Sprintf("unknown field type: %v", f))
	}

	if err != nil {
		enc.AddString(fmt.Sprintf("%sError", f.Key), err.Error())
	}
}

// Equals returns whether two fields are equal. For non-primitive types such as
// errors, marshalers, or reflect types, it uses reflect.DeepEqual.
func (f Field) Equals(other Field) bool {
	if f.Type != other.Type {
		return false
	}
	if f.Key != other.Key {
		return false
	}

	switch f.Type {
	case BinaryType, ByteStringType:
Repository: uber-go/zap
File: zapcore/field_test.go (Go)

Code before:
	return fmt.Sprintf("%d users", int(u))
}

func (u users) MarshalLogObject(enc ObjectEncoder) error {
	if int(u) < 0 {
		return errors.New("too few users")
	}
	enc.AddInt("users", int(u))
	return nil
}

func (u users) MarshalLogArray(enc ArrayEncoder) error {
	if int(u) < 0 {
		return errors.New("too few users")
	}
	for i := 0; i < int(u); i++ {
		enc.AppendString("user")
	}
	return nil
}

type obj struct {
	kind int
}

func (o *obj) String() string {
	if o == nil {
		return "nil obj"
	}

	if o.kind == 1 {
		panic("panic with string")
	} else if o.kind == 2 {
		panic(errors.New("panic with error"))
	} else if o.kind == 3 {
		// recursive - causes a panic when trying to print the panic value
		panic(o)
	}

	return "obj"
}

func TestUnknownFieldType(t *testing.T) {
	unknown := Field{Key: "k", String: "foo"}
	assert.Equal(t, UnknownType, unknown.Type, "Expected zero value of FieldType to be UnknownType.")
	assert.Panics(t, func() {
		unknown.AddTo(NewMapObjectEncoder())
	}, "Expected using a field with unknown type to panic.")
}

func TestFieldAddingError(t *testing.T) {

Review comment (suggestion):
let's add a custom object type which mimics production use-cases: we have a `String` method which accesses some field, but it causes a panic because the object is nil.

Code after:
	return fmt.Sprintf("%d users", int(u))
}

func (u users) MarshalLogObject(enc ObjectEncoder) error {
	if int(u) < 0 {
		return errors.New("too few users")
	}
	enc.AddInt("users", int(u))
	return nil
}

func (u users) MarshalLogArray(enc ArrayEncoder) error {
	if int(u) < 0 {
		return errors.New("too few users")
	}
	for i := 0; i < int(u); i++ {
		enc.AppendString("user")
	}
	return nil
}

type obj struct {
	kind int
}

func (o *obj) String() string {
	if o == nil {
		return "nil obj"
	}

	if o.kind == 1 {
		panic("panic with string")
	} else if o.kind == 2 {
		panic(errors.New("panic with error"))
	} else if o.kind == 3 {
		// panic with an arbitrary object that causes a panic itself
		// when being converted to a string
		panic((*url.URL)(nil))
	}

	return "obj"
}

func TestUnknownFieldType(t *testing.T) {
	unknown := Field{Key: "k", String: "foo"}
	assert.Equal(t, UnknownType, unknown.Type, "Expected zero value of FieldType to be UnknownType.")
	assert.Panics(t, func() {
		unknown.AddTo(NewMapObjectEncoder())
	}, "Expected using a field with unknown type to panic.")
}
Repository: uber-go/zap
File: zapcore/field.go (Go)

Code before:
		enc.AddInt8(f.Key, int8(f.Integer))
	case StringType:
		enc.AddString(f.Key, f.String)
	case TimeType:
		if f.Interface != nil {
			enc.AddTime(f.Key, time.Unix(0, f.Integer).In(f.Interface.(*time.Location)))
		} else {
			// Fall back to UTC if location is nil.
			enc.AddTime(f.Key, time.Unix(0, f.Integer))
		}
	case Uint64Type:
		enc.AddUint64(f.Key, uint64(f.Integer))
	case Uint32Type:
		enc.AddUint32(f.Key, uint32(f.Integer))
	case Uint16Type:
		enc.AddUint16(f.Key, uint16(f.Integer))
	case Uint8Type:
		enc.AddUint8(f.Key, uint8(f.Integer))
	case UintptrType:
		enc.AddUintptr(f.Key, uintptr(f.Integer))
	case ReflectType:
		err = enc.AddReflected(f.Key, f.Interface)
	case NamespaceType:
		enc.OpenNamespace(f.Key)
	case StringerType:
		encodeStringer(f.Key, f.Interface, enc)
	case ErrorType:
		encodeError(f.Key, f.Interface.(error), enc)
	case SkipType:
		break
	default:
		panic(fmt.Sprintf("unknown field type: %v", f))
	}

	if err != nil {
		enc.AddString(fmt.Sprintf("%sError", f.Key), err.Error())
	}
}

// Equals returns whether two fields are equal. For non-primitive types such as
// errors, marshalers, or reflect types, it uses reflect.DeepEqual.
func (f Field) Equals(other Field) bool {
	if f.Type != other.Type {
		return false
	}
	if f.Key != other.Key {
		return false
	}

	switch f.Type {
	case BinaryType, ByteStringType:

Review comment (suggestion):
rather than duplicating the logic of how errors are encoded, this should probably be `err = encodeStringer(..)` and the panic handling should return an error

Code after:
		enc.AddInt8(f.Key, int8(f.Integer))
	case StringType:
		enc.AddString(f.Key, f.String)
	case TimeType:
		if f.Interface != nil {
			enc.AddTime(f.Key, time.Unix(0, f.Integer).In(f.Interface.(*time.Location)))
		} else {
			// Fall back to UTC if location is nil.
			enc.AddTime(f.Key, time.Unix(0, f.Integer))
		}
	case Uint64Type:
		enc.AddUint64(f.Key, uint64(f.Integer))
	case Uint32Type:
		enc.AddUint32(f.Key, uint32(f.Integer))
	case Uint16Type:
		enc.AddUint16(f.Key, uint16(f.Integer))
	case Uint8Type:
		enc.AddUint8(f.Key, uint8(f.Integer))
	case UintptrType:
		enc.AddUintptr(f.Key, uintptr(f.Integer))
	case ReflectType:
		err = enc.AddReflected(f.Key, f.Interface)
	case NamespaceType:
		enc.OpenNamespace(f.Key)
	case StringerType:
		err = encodeStringer(f.Key, f.Interface, enc)
	case ErrorType:
		encodeError(f.Key, f.Interface.(error), enc)
	case SkipType:
		break
	default:
		panic(fmt.Sprintf("unknown field type: %v", f))
	}

	if err != nil {
		enc.AddString(fmt.Sprintf("%sError", f.Key), err.Error())
	}
}

// Equals returns whether two fields are equal. For non-primitive types such as
// errors, marshalers, or reflect types, it uses reflect.DeepEqual.
func (f Field) Equals(other Field) bool {
	if f.Type != other.Type {
		return false
	}
	if f.Key != other.Key {
		return false
	}

	switch f.Type {
	case BinaryType, ByteStringType:
Repository: uber-go/zap
File: zapcore/field.go (Go)

Code before:
func (f Field) Equals(other Field) bool {
	if f.Type != other.Type {
		return false
	}
	if f.Key != other.Key {
		return false
	}

	switch f.Type {
	case BinaryType, ByteStringType:
		return bytes.Equal(f.Interface.([]byte), other.Interface.([]byte))
	case ArrayMarshalerType, ObjectMarshalerType, ErrorType, ReflectType:
		return reflect.DeepEqual(f.Interface, other.Interface)
	default:
		return f == other
	}
}

func addFields(enc ObjectEncoder, fields []Field) {
	for i := range fields {
		fields[i].AddTo(enc)
	}
}

func encodeStringer(key string, stringer interface{}, enc ObjectEncoder) {
	defer catchPanic(key, enc)
	enc.AddString(key, stringer.(fmt.Stringer).String())
}

func catchPanic(key string, enc ObjectEncoder) {
	if err := recover(); err != nil {
		var msg string

		switch v := err.(type) {
		case error:
			msg = v.Error()
		case string:
			msg = v
		default:
			msg = "<unknown>"
		}

		enc.AddString(fmt.Sprintf("%sError", key), msg)
	}
}


Review comment (bug):
ok with inlining `catchPanic`, especially since we'll need to modify the returned error here (if we make `encodeStringer` return `error`:

```go
func encodeStringer(key string, stringer interface{}, enc ObjectEncoder) (retError error) {
  defer func() 
    if err := recover(); err != nil {
      // massage error
      retError = err
    }
  }()

  enc.AddString(key, stringer.(fmt.Stringer).String())
```

Code after:
func (f Field) Equals(other Field) bool {
	if f.Type != other.Type {
		return false
	}
	if f.Key != other.Key {
		return false
	}

	switch f.Type {
	case BinaryType, ByteStringType:
		return bytes.Equal(f.Interface.([]byte), other.Interface.([]byte))
	case ArrayMarshalerType, ObjectMarshalerType, ErrorType, ReflectType:
		return reflect.DeepEqual(f.Interface, other.Interface)
	default:
		return f == other
	}
}

func addFields(enc ObjectEncoder, fields []Field) {
	for i := range fields {
		fields[i].AddTo(enc)
	}
}

func encodeStringer(key string, stringer interface{}, enc ObjectEncoder) (err error) {
	defer func() {
		if v := recover(); v != nil {
			err = fmt.Errorf("PANIC=%v", v)
		}
	}()

	enc.AddString(key, stringer.(fmt.Stringer).String())
	return
}