search

charmbracelet / log

A minimal, colorful Go logging library 🪵
MIT License
2.4k stars 67 forks source link

feat: support slog attributes #127

Closed op closed 5 months ago

op commented 6 months ago

This adds support for slog attributes and fixes #119.

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 85.71429% with 6 lines in your changes missing coverage. Please review.

Project coverage is 82.63%. Comparing base (2338a13) to head (6594782). Report is 23 commits behind head on main.

Files Patch % Lines
json.go 85.36% 6 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #127 +/- ## ========================================== + Coverage 80.92% 82.63% +1.71% ========================================== Files 11 11 Lines 739 714 -25 ========================================== - Hits 598 590 -8 + Misses 126 108 -18 - Partials 15 16 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

aymanbagabas commented 6 months ago

This adds support for slog attributes and fixes #119.

TODO

Current PR assume we're on Go 1.21. Open question:

  1. Upgrade to require Go 1.21. This would allow merging of logger.go and logger_121.go and simplify existing code base.
  2. Split all slog stuff out into json_go121.go and further complicate code base.

@aymanbagabas what do you think?

I would say keep supporting Go 1.19 and split code. We can use type aliases to simplify things. For example, slog.Attr can be defined as type slogAttr = slog.Attr for both Go 1.19 and Go 1.21. The same goes for the other imported slog types. This way we don't need to import slog in json.go.

op commented 6 months ago

@aymanbagabas very cool idea! that worked pretty nice.

op commented 5 months ago

@aymanbagabas hi, did you see that all comments has been addressed? :)

aymanbagabas commented 5 months ago

Hey @op! Thanks for addressing the comments. I tried running the example from #119, but I think the implementation still doesn't support arbitrary maps, which is the whole point of the issue.

op commented 5 months ago

Hey @op! Thanks for addressing the comments. I tried running the example from #119, but I think the implementation still doesn't support arbitrary maps, which is the whole point of the issue.

No worries. Good catch @aymanbagabas! Didn't realise the slog Handle function worked differently. It was previously calling attr.Value.String(). I dropped the .String() call to allow the unmodified slog.Value flow through all formatters. For the JSON formatter, I've added a check to make sure it's output is identical in "slog mode" too now.

Fixing the text handler seem like a future improvement to me. Sorry, I don't have time to look at that.

op commented 5 months ago

@aymanbagabas ping! :penguin:

op commented 5 months ago

@aymanbagabas seems like govulncheck doesn't work with 1.19. Looks like https://github.com/golang/vuln/commit/745db65f35062023c5994fceb1931ef839777007 introduces slices.

aymanbagabas commented 5 months ago

@aymanbagabas seems like govulncheck doesn't work with 1.19. Looks like golang/vuln@745db65 introduces slices.

Hmm, seems like golang/vuln needs to update their go.mod

Anyway, this looks good, thanks again @op!