Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

saurabh95
Copy link
Contributor

@saurabh95 saurabh95 commented Aug 22, 2022

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
Copy link

CLAassistant commented Aug 22, 2022

CLA assistant check
All committers have signed the CLA.

@zmanji
Copy link

zmanji commented Aug 22, 2022

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
Copy link

codecov bot commented Aug 22, 2022

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

Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice. pretty straightforward.

CC @sywhang @mway

give Field
want []any
}{
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +130 to +133
// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -179,3 +179,48 @@ func TestObjectsAndObjectValues_marshalError(t *testing.T) {
})
}
}

type stringerObject struct{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: lint

Suggested change
type stringerObject struct{
type stringerObject struct {

Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@abhinav abhinav merged commit 23d6cc7 into uber-go:master Aug 23, 2022
@phenpessoa
Copy link

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
Copy link
Member

mway commented Aug 25, 2022

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
Copy link

That makes perfect sense. Thanks for the explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants