-
Notifications
You must be signed in to change notification settings - Fork 313
change: replace log with log/slog for logging #1280
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
Conversation
ccoVeille
left a comment
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 👍
Only minor feedbacks and a question
logging/logger.go
Outdated
| debugModeEnabled := os.Getenv("DEBUG") != "" | ||
| if !debugModeEnabled { | ||
| // by default, suppress all logging output | ||
| return slog.New(slog.NewTextHandler(io.Discard, nil)), nil |
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.
BTW, is there a linter about slog (not necessarily in revive) about replacing slog.NewTextHandler(io.Discard, nil) ?
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.
I think no, it's not.
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.
Maybe we should think about it
What would be the best candidate? Revive?
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.
Perhaps it's go-critic
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.
BTW, is there a linter about slog (not necessarily in revive) about replacing
slog.NewTextHandler(io.Discard, nil)for Go 1.24+ ?
Perhaps it's go-critic
What do you think about this Oleg ?
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.
Yep, this can be easily done via ruleguard rule. Feel free to submit an issue (or even a PR).
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 for confirmation
It can be done here then:
https://github.com/go-critic/go-critic/blob/master/checkers%2Frules%2Frules.go
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.
I consider sloglint as a possible candidate also
https://github.com/go-simpler/sloglint
@tmzane do you think there could be an interest in sloglint to report people using
slog.NewTextHandler(io.Discard, nil) for Go 1.24+, while slog.DiscardHandler is available
Apparently, it's massively used
If you have interest for this, I will open an issue in your project
If you don't, I will open an issue in go-critric and will code it I think.
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.
Hi, I don't mind adding it to sloglint as in slog-focused linter, seems like a useful and easy-to-implement check. Feel free to open an issue, 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.
|
Thank you! |
* change: replace log with log/slog for logging * add TODO about slog.DiscardHandler
This PR replaces usages of
logtolog/slogas discussed in #1278.Now, the logger outputs to
stderrandrevive.logsimultaneously ifDEBUGenvironment variable is defined and not empty.Example:
For discarding output, I use
slog.NewTextHandler(io.Discard, nil)becauseslog.DiscardHandlerexist only in Go 1.24.