-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Introduce Str + Strs generic string helpers #1281
Conversation
Implements uber-go#1275 by adding new `zapfield.Str` and `zapfield.Strs` types, which operate on generic string-like types and use the underlying `zap.String` and `zap.Strings` codepaths. In uber-go#1275, the decision was to put these in a separate `zap/exp/zapfield` package and I didn't see an existing one, so I made a new `zapfield` package.
Codecov Report
@@ Coverage Diff @@
## master #1281 +/- ##
==========================================
+ Coverage 97.93% 98.08% +0.15%
==========================================
Files 50 50
Lines 3240 3240
==========================================
+ Hits 3173 3178 +5
+ Misses 57 53 -4
+ Partials 10 9 -1 see 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
This is just a small nit from #1281 - I think the expected and got fields of the test case structs are flipped.
Implements #1275 by adding new
zapfield.Str
andzapfield.Strs
types, which operate on generic string-like types and use the underlyingzap.String
andzap.Strings
codepaths.In #1275, the decision was to put these in a separate
zap/exp/zapfield
package and I didn't see an existing one, so I made a newzapfield
package.Since this is a separate package, I had to copy a few test + non-test helpers here. If that's undesirable, I can export them in a shared helper package somewhere and have both places use that