Add Stringers field constructor for array of objects implementing fmt.Stringer method

[saurabh95]

Add a new Zap field constructor that encodes an array of zero or more
objects which implement fmt.Stringer interface into a Zap array.

Usage:

// type Request struct{ ... }
// func (a Request) String() string
//
// var requests []Request = ...
// logger.Info("sending requests", zap.Stringers("requests", requests))

Credits: @zmanji, @abhinav for the suggestions

[CLAassistant]

All committers have signed the CLA.

[zmanji]

This looks good to me, I wonder if we should add some sort of benchmark to show the benefit of this, since I believe the only way before was to use zap.Any.

[codecov]

Codecov Report

Merging #1155 (e3f7e7a) into master (1e46f5e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1155   +/-   ##
=======================================
  Coverage   98.33%   98.33%           
=======================================
  Files          49       49           
  Lines        2160     2163    +3     
=======================================
+ Hits         2124     2127    +3     
  Misses         28       28           
  Partials        8        8           

Impacted Files	Coverage Δ
array_go118.go	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[abhinav]

nice. pretty straightforward.

CC @sywhang @mway

[abhinav]

Can we also add a test for a []fmt.Stringer? That should also work.

[abhinav]

Same caveat as Objects applies here. We should add a comment about it:

// Note that these objects must implement fmt.Stringer directly.
// That is, if you're trying to marshal a []Request, the String method
// must be declared on the Request type, not its pointer (*Request).

In a future change, we can add an ObjectValues equivalent if necessary that will let the String method be on *Request for a []Request.

[sywhang]

👍 Thanks for the contribution. Changes LGTM modulo @abhinav's comments and a lint failure

[sywhang]

nit: lint

Suggested change

	type stringerObject struct{
	type stringerObject struct {

[abhinav]

Thanks!

[phenpessoa]

Hi! I'm wondering if there was a specific reason as to why generics were used in this. Wouldn't this accomplish the same result?

func Stringers(key string, values []fmt.Stringer) zap.Field {
	return zap.Array(key, stringers(values))
}

type stringers []fmt.Stringer

func (os stringers) MarshalLogArray(arr zapcore.ArrayEncoder) error {
	for _, o := range os {
		arr.AppendString(o.String())
	}
	return nil
}

Thanks!

[mway]

Hi! I'm wondering if there was a specific reason as to why generics were used in this. Wouldn't this accomplish the same result?

func Stringers(key string, values []fmt.Stringer) zap.Field {
	return zap.Array(key, stringers(values))
}

type stringers []fmt.Stringer

func (os stringers) MarshalLogArray(arr zapcore.ArrayEncoder) error {
	for _, o := range os {
		arr.AppendString(o.String())
	}
	return nil
}

Thanks!

You can't implicitly convert between []T and []InterfaceThatTImplements by default (you have to allocate storage and copy the contents). So if you have a type Foo that implements fmt.Stringer:

foos := []Foo{ /* ... */ }

We then have to handle it as follows (without generics):

func Stringers(key string, values []fmt.Stringer) zap.Field {
  // ...
}

// Stringers(foos) <- does not compile

converted := make([]fmt.Stringer, len(foos))
for i, f := range foos {
  converted[i] = f // converts type `Foo` to `fmt.Stringer`
}

Stringers(converted) // compiles

Whereas with generics, we can handle both []fmt.Stringer and []T (where T implements fmt.Stringer):

func Stringers[T fmt.Stringer](key string, values []T) zap.Field {
  // ...
}

var (
  a = []fmt.Stringer{ /* ... */ }
  b = []Foo{ /* ... */ }
)

Stringers(a) // compiles
Stringers(b) // compiles

Thus the generic version gives callers more flexibility by (1) letting them use wider types than fmt.Stringer in []T and (2) not requiring conversions.

[phenpessoa]

That makes perfect sense. Thanks for the explanation!