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

fix: RoutableHandler to return handler's Enabled #4

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

akijakya
Copy link
Contributor

@akijakya akijakya commented Oct 4, 2023

It seems to make sense for the RoutableHandler's Enabled method to return its handlers Enabled method instead of true.

It also fixes a bug when if a router's matcher matches the log level, the minimum log level set through &slog.HandlerOptions{} ends up being ignored (example below).

levelFilter := func(levels ...slog.Level) func(ctx context.Context, r slog.Record) bool {
    return func(ctx context.Context, r slog.Record) bool {
        return slices.Contains(levels, r.Level)
    }
}

logger := slog.New(
    slogmulti.Router().
        Add(
            slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelWarn}),
            levelFilter(slog.LevelWarn, slog.LevelError)).
        Add(
            slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelInfo}),
            levelFilter(slog.LevelDebug, slog.LevelInfo)). // before the fix, debug level logs were also sent to stdout
        Handler(),
)

@samber
Copy link
Owner

samber commented Oct 5, 2023

Hi @akijakya and thanks for the report.

I definitely agree with this!

BTW, I wonder why slog does not send slog.Record to Enabled(). It would be the right place to check filters, instead of Handle().

Anyway. Let's merge. -> v1.0.2

@codecov-commenter
Copy link

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (f892b89) 0.00% compared to head (5211a5b) 0.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@          Coverage Diff          @@
##            main      #4   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files         11      11           
  Lines        248     248           
=====================================
  Misses       248     248           
Flag Coverage Δ
unittests 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
router.go 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samber samber merged commit 7cac962 into samber:main Oct 5, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants