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

slog: Don't log if not enabled at level #103

Merged
merged 3 commits into from
Feb 5, 2024

Conversation

imjasonh
Copy link
Contributor

@imjasonh imjasonh commented Feb 5, 2024

This is a very naive first attempt to fix a potential bug I noticed in this package's integration with slog.

When I used

slog.SetDefault(slog.New(log.NewWithOptions(out, log.Options{Level: WarnLevel})))

I was still seeing INFO and DEBU logs, and I couldn't figure out why. I think this is the cause -- the slog Handle method isn't consulting Enabled before handling. slog's builtin Text and JSON handlers consult the enabled level before logging.

Patching this locally fixed the issue for me, so I thought I'd contribute it. Please let me know if this is on the wrong track.

logger_121.go Outdated Show resolved Hide resolved
@aymanbagabas aymanbagabas added the bug Something isn't working label Feb 5, 2024
@aymanbagabas
Copy link
Member

@imjasonh Good catch! Looks good, thanks!

@aymanbagabas aymanbagabas merged commit 7a3834f into charmbracelet:main Feb 5, 2024
11 checks passed
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 this pull request may close these issues.

2 participants