Add Stringers field constructor for array of objects implementing fmt.Stringer method#1155
Conversation
|
|
|
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 |
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
126f793 to
2101cab
Compare
| give Field | ||
| want []any | ||
| }{ | ||
| { |
There was a problem hiding this comment.
Can we also add a test for a []fmt.Stringer? That should also work.
| // Stringers constructs a field with the given key, holding a list of the | ||
| // output provided by the value's String method | ||
| // | ||
| // Given an object that implements String on the value receiver, you |
There was a problem hiding this comment.
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.
array_go118_test.go
Outdated
| } | ||
| } | ||
|
|
||
| type stringerObject struct{ |
There was a problem hiding this comment.
nit: lint
| type stringerObject struct{ | |
| type stringerObject struct { |
2101cab to
e3f7e7a
Compare
|
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 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) // compilesWhereas with generics, we can handle both func Stringers[T fmt.Stringer](key string, values []T) zap.Field {
// ...
}
var (
a = []fmt.Stringer{ /* ... */ }
b = []Foo{ /* ... */ }
)
Stringers(a) // compiles
Stringers(b) // compilesThus the generic version gives callers more flexibility by (1) letting them use wider types than |
|
That makes perfect sense. Thanks for the explanation! |
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