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

Code before:

func (enc *jsonEncoder) AddFloat64(key string, val float64) {
	enc.addKey(key)
	enc.AppendFloat64(val)
}

func (enc *jsonEncoder) AddFloat32(key string, val float32) {
	enc.addKey(key)
	enc.AppendFloat32(val)
}

func (enc *jsonEncoder) AddInt64(key string, val int64) {
	enc.addKey(key)
	enc.AppendInt64(val)
}

func (enc *jsonEncoder) resetReflectBuf() {
	if enc.reflectBuf == nil {
		enc.reflectBuf = bufferpool.Get()
		// If no EncoderConfig.NewReflectedEncoder is provided by the user, then use default
		var newReflectedEncoder func(io.Writer) ReflectedEncoder
		if enc.NewReflectedEncoder == nil {
			newReflectedEncoder = GetDefaultReflectedEncoder()
		} else {
			newReflectedEncoder = enc.NewReflectedEncoder
		}
		enc.reflectEnc = newReflectedEncoder(enc.reflectBuf)
	} else {
		enc.reflectBuf.Reset()
	}
}

var nullLiteralBytes = []byte("null")

// Only invoke the standard JSON encoder if there is actually something to
// encode; otherwise write JSON null literal directly.
func (enc *jsonEncoder) encodeReflected(obj interface{}) ([]byte, error) {
	if obj == nil {
		return nullLiteralBytes, nil
	}
	enc.resetReflectBuf()
	if err := enc.reflectEnc.Encode(obj); err != nil {
		return nil, err
	}
	enc.reflectBuf.TrimNewline()
	return enc.reflectBuf.Bytes(), nil
}

func (enc *jsonEncoder) AddReflected(key string, obj interface{}) error {
	valueBytes, err := enc.encodeReflected(obj)
	if err != nil {

Review comment (performance):
We should do this in `NewJSONEncoder`? We can set `NewReflectedEncoder` in there once if it's nil, and then we don't have to check it on the hot path.

Code after:
func (enc *jsonEncoder) AddDuration(key string, val time.Duration) {
	enc.addKey(key)
	enc.AppendDuration(val)
}

func (enc *jsonEncoder) AddFloat64(key string, val float64) {
	enc.addKey(key)
	enc.AppendFloat64(val)
}

func (enc *jsonEncoder) AddFloat32(key string, val float32) {
	enc.addKey(key)
	enc.AppendFloat32(val)
}

func (enc *jsonEncoder) AddInt64(key string, val int64) {
	enc.addKey(key)
	enc.AppendInt64(val)
}

func (enc *jsonEncoder) resetReflectBuf() {
	if enc.reflectBuf == nil {
		enc.reflectBuf = bufferpool.Get()
		enc.reflectEnc = enc.NewReflectedEncoder(enc.reflectBuf)
	} else {
		enc.reflectBuf.Reset()
	}
}

var nullLiteralBytes = []byte("null")

// Only invoke the standard JSON encoder if there is actually something to
// encode; otherwise write JSON null literal directly.
func (enc *jsonEncoder) encodeReflected(obj interface{}) ([]byte, error) {
	if obj == nil {
		return nullLiteralBytes, nil
	}
	enc.resetReflectBuf()
	if err := enc.reflectEnc.Encode(obj); err != nil {
		return nil, err
	}
	enc.reflectBuf.TrimNewline()
	return enc.reflectBuf.Bytes(), nil
}

func (enc *jsonEncoder) AddReflected(key string, obj interface{}) error {
	valueBytes, err := enc.encodeReflected(obj)
	if err != nil {
		return err
	}
	enc.addKey(key)
Repository: uber-go/zap
File: zapcore/reflected_encoder.go (Go)

Code before:
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

package zapcore

import (
	"encoding/json"
	"io"
)

// A ReflectedEncoder handles the serialization of Field which have FieldType = ReflectType and writes them to the underlying data stream
// The underlying data stream is provided by zap during initialization. See EncoderConfig.NewReflectedEncoder
type ReflectedEncoder interface {
	// Encode encodes and writes to the underlying data stream
	Encode(interface{}) error
}

func GetDefaultReflectedEncoder() func(writer io.Writer) ReflectedEncoder {
	return func(writer io.Writer) ReflectedEncoder {
		stdJsonEncoder := json.NewEncoder(writer)
		// For consistency with our custom JSON encoder.
		stdJsonEncoder.SetEscapeHTML(false)
		return &stdReflectedEncoder{
			encoder: stdJsonEncoder,
		}
	}
}

type stdReflectedEncoder struct {
	encoder *json.Encoder
}

func (enc *stdReflectedEncoder) Encode(obj interface{}) error {
	return enc.encoder.Encode(obj)
}


Review comment (security):
We would prefer not to export this function. We also don't need the wrapper type because json.Encoder satisfies the ReflectedEncoder interface.

So something like the following should suffice:

```
func defaultReflectedEncoder(w io.Writer) ReflectedEncoder {
  enc := json.NewEncoder(w)
  enc.SetEscapeHTML(false)
  return enc
}
```

Code after:
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

package zapcore

import (
	"encoding/json"
	"io"
)

// ReflectedEncoder serializes log fields that can't be serialized with Zap's
// JSON encoder. These have the ReflectType field type.
// Use EncoderConfig.NewReflectedEncoder to set this.
type ReflectedEncoder interface {
	// Encode encodes and writes to the underlying data stream.
	Encode(interface{}) error
}

func defaultReflectedEncoder(w io.Writer) ReflectedEncoder {
	enc := json.NewEncoder(w)
	// For consistency with our custom JSON encoder.
	enc.SetEscapeHTML(false)
	return enc
}
Repository: uber-go/zap
File: zapcore/json_encoder_test.go (Go)

Code before:
			buf, err := enc.EncodeEntry(zapcore.Entry{
				Level:      zapcore.DebugLevel,
				Time:       time.Now(),
				LoggerName: "mylogger",
				Message:    "things happened",
			}, []zapcore.Field{tt.field})
			if assert.NoError(t, err, "Unexpected JSON encoding error.") {
				assert.JSONEq(t, tt.expected, buf.String(), "Incorrect encoded JSON entry.")
			}

			buf.Free()
		})
	}
}

// Encodes any object into empty json '{}'
type emptyReflectedEncoder struct {
	writer io.Writer
}

func (enc *emptyReflectedEncoder) Encode(obj interface{}) error {
	_, err := enc.writer.Write([]byte("{}"))
	return err
}

func TestJSONCustomReflectionEncoder(t *testing.T) {
	tests := []struct {
		name     string
		field    zapcore.Field
		expected string
	}{
		{
			name: "encode custom map object",
			field: zapcore.Field{
				Key:  "data",
				Type: zapcore.ReflectType,
				Interface: map[string]interface{}{
					"foo": "hello",
					"bar": 1111,
				},
			},
			expected: `{"data":{}}`,
		},
		{
			name: "encode nil object",
			field: zapcore.Field{
				Key:  "data",
				Type: zapcore.ReflectType,
			},
			expected: `{"data":null}`,
		},

Review comment (suggestion):
minor nit: 
```suggestion
func TestJSONCustomReflectedEncoder(t *testing.T) {
```

Code after:
			buf, err := enc.EncodeEntry(zapcore.Entry{
				Level:      zapcore.DebugLevel,
				Time:       time.Now(),
				LoggerName: "mylogger",
				Message:    "things happened",
			}, []zapcore.Field{tt.field})
			if assert.NoError(t, err, "Unexpected JSON encoding error.") {
				assert.JSONEq(t, tt.expected, buf.String(), "Incorrect encoded JSON entry.")
			}

			buf.Free()
		})
	}
}

// Encodes any object into empty json '{}'
type emptyReflectedEncoder struct {
	writer io.Writer
}

func (enc *emptyReflectedEncoder) Encode(obj interface{}) error {
	_, err := enc.writer.Write([]byte("{}"))
	return err
}

func TestJSONCustomReflectedEncoder(t *testing.T) {
	tests := []struct {
		name     string
		field    zapcore.Field
		expected string
	}{
		{
			name: "encode custom map object",
			field: zapcore.Field{
				Key:  "data",
				Type: zapcore.ReflectType,
				Interface: map[string]interface{}{
					"foo": "hello",
					"bar": 1111,
				},
			},
			expected: `{"data":{}}`,
		},
		{
			name: "encode nil object",
			field: zapcore.Field{
				Key:  "data",
				Type: zapcore.ReflectType,
			},
			expected: `{"data":null}`,
		},
Repository: uber-go/zap
File: zapcore/json_encoder_test.go (Go)

Code before:
					"bar": 1111,
				},
			},
			expected: `{"data":{}}`,
		},
		{
			name: "encode nil object",
			field: zapcore.Field{
				Key:  "data",
				Type: zapcore.ReflectType,
			},
			expected: `{"data":null}`,
		},
	}

	enc := zapcore.NewJSONEncoder(zapcore.EncoderConfig{
		NewReflectedEncoder: func(writer io.Writer) zapcore.ReflectedEncoder {
			return &emptyReflectedEncoder{
				writer: writer,
			}
		},
	})

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			buf, err := enc.EncodeEntry(zapcore.Entry{
				Level:      zapcore.DebugLevel,
				Time:       time.Now(),
				LoggerName: "logger",
				Message:    "things happened",
			}, []zapcore.Field{tt.field})
			if assert.NoError(t, err, "Unexpected JSON encoding error.") {
				assert.JSONEq(t, tt.expected, buf.String(), "Incorrect encoded JSON entry.")
			}
			buf.Free()
		})
	}
}


Review comment (suggestion):
```suggestion
	for _, tt := range tests {
	        tt := tt
		t.Run(tt.name, func(t *testing.T) {
		        t.Parallel()
			buf, err := enc.EncodeEntry(zapcore.Entry{
```

Code after:
					"bar": 1111,
				},
			},
			expected: `{"data":{}}`,
		},
		{
			name: "encode nil object",
			field: zapcore.Field{
				Key:  "data",
				Type: zapcore.ReflectType,
			},
			expected: `{"data":null}`,
		},
	}

	for _, tt := range tests {
		tt := tt
		t.Run(tt.name, func(t *testing.T) {
			t.Parallel()

			enc := zapcore.NewJSONEncoder(zapcore.EncoderConfig{
				NewReflectedEncoder: func(writer io.Writer) zapcore.ReflectedEncoder {
					return &emptyReflectedEncoder{
						writer: writer,
					}
				},
			})

			buf, err := enc.EncodeEntry(zapcore.Entry{
				Level:      zapcore.DebugLevel,
				Time:       time.Now(),
				LoggerName: "logger",
				Message:    "things happened",
			}, []zapcore.Field{tt.field})
			if assert.NoError(t, err, "Unexpected JSON encoding error.") {
				assert.JSONEq(t, tt.expected, buf.String(), "Incorrect encoded JSON entry.")
			}
			buf.Free()
		})
	}
}
Repository: uber-go/zap
File: zapcore/level.go (Go)

Code before:
	// WarnLevel logs are more important than Info, but don't need individual
	// human review.
	WarnLevel
	// ErrorLevel logs are high-priority. If an application is running smoothly,
	// it shouldn't generate any error-level logs.
	ErrorLevel
	// DPanicLevel logs are particularly important errors. In development the
	// logger panics after writing the message.
	DPanicLevel
	// PanicLevel logs a message, then panics.
	PanicLevel
	// FatalLevel logs a message, then calls os.Exit(1).
	FatalLevel

	_minLevel = DebugLevel
	_maxLevel = FatalLevel
)

// UnmarshalLevel unmarshals a level based on the lower-case or all-caps ASCII
// representation of the log level. If the provided ASCII representation is
// invalid an error is returned.
//
// This is particularly useful when dealing with text input to configure log
// levels.
func UnmarshalLevel(text string) (Level, error) {
	level := DebugLevel
	err := level.UnmarshalText([]byte(text))
	if err != nil {
		return level, err
	}
	return level, nil
}

// String returns a lower-case ASCII representation of the log level.
func (l Level) String() string {
	switch l {
	case DebugLevel:
		return "debug"
	case InfoLevel:
		return "info"
	case WarnLevel:
		return "warn"
	case ErrorLevel:
		return "error"
	case DPanicLevel:
		return "dpanic"
	case PanicLevel:
		return "panic"
	case FatalLevel:
		return "fatal"
	default:

Review comment (suggestion):
We should use the zero value as the default here

```suggestion
	var level Level
```

Code after:
	// WarnLevel logs are more important than Info, but don't need individual
	// human review.
	WarnLevel
	// ErrorLevel logs are high-priority. If an application is running smoothly,
	// it shouldn't generate any error-level logs.
	ErrorLevel
	// DPanicLevel logs are particularly important errors. In development the
	// logger panics after writing the message.
	DPanicLevel
	// PanicLevel logs a message, then panics.
	PanicLevel
	// FatalLevel logs a message, then calls os.Exit(1).
	FatalLevel

	_minLevel = DebugLevel
	_maxLevel = FatalLevel
)

// ParseLevel parses a level based on the lower-case or all-caps ASCII
// representation of the log level. If the provided ASCII representation is
// invalid an error is returned.
//
// This is particularly useful when dealing with text input to configure log
// levels.
func ParseLevel(text string) (Level, error) {
	var level Level
	err := level.UnmarshalText([]byte(text))
	return level, err
}

// String returns a lower-case ASCII representation of the log level.
func (l Level) String() string {
	switch l {
	case DebugLevel:
		return "debug"
	case InfoLevel:
		return "info"
	case WarnLevel:
		return "warn"
	case ErrorLevel:
		return "error"
	case DPanicLevel:
		return "dpanic"
	case PanicLevel:
		return "panic"
	case FatalLevel:
		return "fatal"
	default:
		return fmt.Sprintf("Level(%d)", l)
	}
}
Repository: uber-go/zap
File: zapcore/level.go (Go)

Code before:
	ErrorLevel
	// DPanicLevel logs are particularly important errors. In development the
	// logger panics after writing the message.
	DPanicLevel
	// PanicLevel logs a message, then panics.
	PanicLevel
	// FatalLevel logs a message, then calls os.Exit(1).
	FatalLevel

	_minLevel = DebugLevel
	_maxLevel = FatalLevel
)

// UnmarshalLevel unmarshals a level based on the lower-case or all-caps ASCII
// representation of the log level. If the provided ASCII representation is
// invalid an error is returned.
//
// This is particularly useful when dealing with text input to configure log
// levels.
func UnmarshalLevel(text string) (Level, error) {
	level := DebugLevel
	err := level.UnmarshalText([]byte(text))
	if err != nil {
		return level, err
	}
	return level, nil
}

// String returns a lower-case ASCII representation of the log level.
func (l Level) String() string {
	switch l {
	case DebugLevel:
		return "debug"
	case InfoLevel:
		return "info"
	case WarnLevel:
		return "warn"
	case ErrorLevel:
		return "error"
	case DPanicLevel:
		return "dpanic"
	case PanicLevel:
		return "panic"
	case FatalLevel:
		return "fatal"
	default:
		return fmt.Sprintf("Level(%d)", l)
	}
}

// CapitalString returns an all-caps ASCII representation of the log level.

Review comment (refactor):
minor change: we can simplify this:
```suggestion
	return level, err
```

Code after:
	ErrorLevel
	// DPanicLevel logs are particularly important errors. In development the
	// logger panics after writing the message.
	DPanicLevel
	// PanicLevel logs a message, then panics.
	PanicLevel
	// FatalLevel logs a message, then calls os.Exit(1).
	FatalLevel

	_minLevel = DebugLevel
	_maxLevel = FatalLevel
)

// ParseLevel parses a level based on the lower-case or all-caps ASCII
// representation of the log level. If the provided ASCII representation is
// invalid an error is returned.
//
// This is particularly useful when dealing with text input to configure log
// levels.
func ParseLevel(text string) (Level, error) {
	var level Level
	err := level.UnmarshalText([]byte(text))
	return level, err
}

// String returns a lower-case ASCII representation of the log level.
func (l Level) String() string {
	switch l {
	case DebugLevel:
		return "debug"
	case InfoLevel:
		return "info"
	case WarnLevel:
		return "warn"
	case ErrorLevel:
		return "error"
	case DPanicLevel:
		return "dpanic"
	case PanicLevel:
		return "panic"
	case FatalLevel:
		return "fatal"
	default:
		return fmt.Sprintf("Level(%d)", l)
	}
}

// CapitalString returns an all-caps ASCII representation of the log level.
func (l Level) CapitalString() string {
	// Printing levels in all-caps is common enough that we should export this
	// functionality.
Repository: uber-go/zap
File: array_go118.go (Go)

Code before:
// If it's on the value, use Objects.
//
// Given an object that implements MarshalLogObject on the pointer receiver,
// you can log a slice of those objects with ObjectValues like so:
//
//  type Request struct{ ... }
//  func (r *Request) MarshalLogObject(enc zapcore.ObjectEncoder) error
//
//  var requests []Request = ...
//  logger.Info("sending requests", zap.ObjectValues("requests", requests))
//
// If instead, you have a slice of pointers of such an object, use the Objects
// field constructor.
//
//  var requests []*Request = ...
//  logger.Info("sending requests", zap.Objects("requests", requests))
func ObjectValues[T any, P objectMarshalerPtr[T]](key string, values []T) Field {
	return Array(key, objectValues[T, P](values))
}

type objectValues[T any, P objectMarshalerPtr[T]] []T

func (os objectValues[T, P]) MarshalLogArray(arr zapcore.ArrayEncoder) error {
	for i := range os {
		// It is necessary for us to explicitly reference the "P" type.
		// We cannot simply pass "&o" to AppendObject because its type
		// is "*T", which the type system does not consider as
		// implementing ObjectMarshaler.
		// Only the type "P" satisfies ObjectMarshaler, which we have
		// to convert "*T" to explicitly.
		var p P = &os[i]
		if err := arr.AppendObject(p); err != nil {
			return err
		}
	}
	return nil
}


Review comment (suggestion):
```suggestion
		// We cannot simply pass "&os[i]" to AppendObject because its type
```

Code after:
// If it's on the value, use Objects.
//
// Given an object that implements MarshalLogObject on the pointer receiver,
// you can log a slice of those objects with ObjectValues like so:
//
//  type Request struct{ ... }
//  func (r *Request) MarshalLogObject(enc zapcore.ObjectEncoder) error
//
//  var requests []Request = ...
//  logger.Info("sending requests", zap.ObjectValues("requests", requests))
//
// If instead, you have a slice of pointers of such an object, use the Objects
// field constructor.
//
//  var requests []*Request = ...
//  logger.Info("sending requests", zap.Objects("requests", requests))
func ObjectValues[T any, P objectMarshalerPtr[T]](key string, values []T) Field {
	return Array(key, objectValues[T, P](values))
}

type objectValues[T any, P objectMarshalerPtr[T]] []T

func (os objectValues[T, P]) MarshalLogArray(arr zapcore.ArrayEncoder) error {
	for i := range os {
		// It is necessary for us to explicitly reference the "P" type.
		// We cannot simply pass "&os[i]" to AppendObject because its type
		// is "*T", which the type system does not consider as
		// implementing ObjectMarshaler.
		// Only the type "P" satisfies ObjectMarshaler, which we have
		// to convert "*T" to explicitly.
		var p P = &os[i]
		if err := arr.AppendObject(p); err != nil {
			return err
		}
	}
	return nil
}
Repository: uber-go/zap
File: logger_test.go (Go)

Code before:
					Context: []Field{},
				}}, logs.AllUntimed(), "unexpected logs")
			})
		})
	}
}

type customWriteHook struct {
	called bool
}

func (h *customWriteHook) OnWrite(_ *zapcore.CheckedEntry, _ []Field) {
	h.called = true
}

func TestLoggerWithFatalHook(t *testing.T) {
	h := &customWriteHook{}
	withLogger(t, InfoLevel, opts(WithFatalHook(h)), func(logger *Logger, logs *observer.ObservedLogs) {
		recovered := make(chan interface{})
		go func() {
			defer func() {
				recovered <- recover()
			}()
			logger.Fatal("")
		}()
		<-recovered
		assert.True(t, h.called)
	})
}

func TestNopLogger(t *testing.T) {
	logger := NewNop()

	t.Run("basic levels", func(t *testing.T) {
		logger.Debug("foo", String("k", "v"))
		logger.Info("bar", Int("x", 42))
		logger.Warn("baz", Strings("ks", []string{"a", "b"}))
		logger.Error("qux", Error(errors.New("great sadness")))
	})

	t.Run("DPanic", func(t *testing.T) {
		logger.With(String("component", "whatever")).DPanic("stuff")
	})

	t.Run("Panic", func(t *testing.T) {
		assert.Panics(t, func() {
			logger.Panic("great sadness")
		}, "Nop logger should still cause panics.")
	})
}


Review comment (suggestion):
The goroutine or channel isn't necessary.
The logger.Fatal won't panic because we're overriding it.

Code after:
					Context: []Field{},
				}}, logs.AllUntimed(), "unexpected logs")
			})
		})
	}
}

type customWriteHook struct {
	called bool
}

func (h *customWriteHook) OnWrite(_ *zapcore.CheckedEntry, _ []Field) {
	h.called = true
}

func TestLoggerWithFatalHook(t *testing.T) {
	var h customWriteHook
	withLogger(t, InfoLevel, opts(WithFatalHook(&h)), func(logger *Logger, logs *observer.ObservedLogs) {
		logger.Fatal("great sadness")
		assert.True(t, h.called)
		assert.Equal(t, 1, logs.FilterLevelExact(FatalLevel).Len())
	})
}

func TestNopLogger(t *testing.T) {
	logger := NewNop()

	t.Run("basic levels", func(t *testing.T) {
		logger.Debug("foo", String("k", "v"))
		logger.Info("bar", Int("x", 42))
		logger.Warn("baz", Strings("ks", []string{"a", "b"}))
		logger.Error("qux", Error(errors.New("great sadness")))
	})

	t.Run("DPanic", func(t *testing.T) {
		logger.With(String("component", "whatever")).DPanic("stuff")
	})

	t.Run("Panic", func(t *testing.T) {
		assert.Panics(t, func() {
			logger.Panic("great sadness")
		}, "Nop logger should still cause panics.")
	})
}

func infoLog(logger *Logger, msg string, fields ...Field) {
	logger.Info(msg, fields...)
}

func infoLogSugared(logger *SugaredLogger, args ...interface{}) {
	logger.Info(args...)