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

Disable zap by default #27

Closed
timonwong opened this issue Aug 29, 2022 · 3 comments · Fixed by #28
Closed

Disable zap by default #27

timonwong opened this issue Aug 29, 2022 · 3 comments · Fixed by #28
Labels
bug Something isn't working

Comments

@timonwong
Copy link
Owner

timonwong commented Aug 29, 2022

By inspecting the zap implementation:

https://github.com/uber-go/zap/blob/0d6a75bccff91a3106f193c9e4e1678738a63f9d/sugar.go#L339-L342

We can see in zap sugar logger, the keys and values can work without pairs (using "Fields" instead)

So in order to reduce the opportunity of false positives, we should disable it by default.

@ttys3 What do you think?

@timonwong timonwong added the bug Something isn't working label Aug 29, 2022
timonwong added a commit to timonwong/golangci-lint that referenced this issue Aug 29, 2022
@ttys3
Copy link
Collaborator

ttys3 commented Aug 29, 2022

Oops

I re-checked the code your point out.

OK. now I got your point.

the code you point out is a special case.

someone use a zap sugared logger and then write zap core Field in it.

like this:

logger, _ := zap.NewProduction()
defer logger.Sync() // flushes buffer, if any

sugar := logger.Sugar()
sugar.Infow("failed to fetch URL",
  zap.String("url", url),
  "attempt", 3,
  "backoff", time.Second,
)

OK, in current linter implemention, this should got false positives

@ttys3
Copy link
Collaborator

ttys3 commented Aug 29, 2022

but I think use a zap Field in a zap sugared logger, this coding style is bad.

if the linter point that out, that should be a good thing ? not a bug.

one should use zap core logger, it they like to write Field, and it has better performance

@timonwong
Copy link
Owner Author

@ttys3 ok, for most use cases our check is just ok. However I'll try to implement a special checker for zap instead, when my hands free

timonwong added a commit that referenced this issue Aug 30, 2022
timonwong added a commit that referenced this issue Aug 30, 2022
timonwong added a commit that referenced this issue Aug 30, 2022
ldez pushed a commit to timonwong/golangci-lint that referenced this issue Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants