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

feat: support slog attributes #127

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

op
Copy link
Contributor

@op op commented May 7, 2024

This adds support for slog attributes and fixes #119.

Copy link

codecov bot commented May 7, 2024

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 ⚠️
Additional details and impacted files
@@            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     

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

@aymanbagabas
Copy link
Member

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 op marked this pull request as ready for review May 7, 2024 15:55
@op op requested a review from aymanbagabas as a code owner May 7, 2024 15:55
@op
Copy link
Contributor Author

op commented May 7, 2024

@aymanbagabas very cool idea! that worked pretty nice.

@op op force-pushed the op/json-order-slog branch 2 times, most recently from 02952f6 to 9e69d85 Compare May 7, 2024 16:44
@op op marked this pull request as draft May 7, 2024 16:52
@op op marked this pull request as ready for review May 7, 2024 16:59
@op op marked this pull request as draft May 7, 2024 17:09
@op op marked this pull request as ready for review May 7, 2024 17:19
logger_121_test.go Show resolved Hide resolved
Copy link
Member

@aymanbagabas aymanbagabas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nit picks and questions, looks good otherwise!

slog_no121.go Outdated Show resolved Hide resolved
logger_121_test.go Show resolved Hide resolved
@op op force-pushed the op/json-order-slog branch 2 times, most recently from 749f61b to ea97607 Compare May 7, 2024 20:08
@op
Copy link
Contributor Author

op commented Jun 2, 2024

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

@aymanbagabas
Copy link
Member

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
Copy link
Contributor Author

op commented Jun 4, 2024

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
Copy link
Contributor Author

op commented Jun 18, 2024

@aymanbagabas ping! 🐧

Copy link
Member

@aymanbagabas aymanbagabas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some last comments before merging

logger_121.go Show resolved Hide resolved
@op
Copy link
Contributor Author

op commented Jun 18, 2024

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

@aymanbagabas
Copy link
Member

@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!

@aymanbagabas aymanbagabas merged commit fb820d2 into charmbracelet:main Jun 18, 2024
11 of 13 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.

improve json handling of slog.Group fields
2 participants