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

Add SlogLoggerAdapter #355

Merged
merged 4 commits into from
Sep 24, 2023
Merged

Conversation

dkotik
Copy link
Contributor

@dkotik dkotik commented Mar 13, 2023

Standard library is going to merge an experimental structured logging package slog soon. Therefore, it is important to support it.

Happy to have this parked until /exp/slog is merged into standard library. I am using the adapter in production already.

Standard library is going to merge an experimental structured logging
package `slog` soon. Therefore, it is important to support it.
@GreyXor
Copy link

GreyXor commented Aug 29, 2023

Thanks @dkotik , works fine for me.
We can change the import to import "log/slog" now that slog is integrated into go 1.21 and can we merge it ?

@dkotik
Copy link
Contributor Author

dkotik commented Aug 29, 2023

@GreyXor ok bumped the version up and switched over to log/slog. I propose that slog should be the default in place of regular log.

@GreyXor
Copy link

GreyXor commented Sep 18, 2023

@dkotik gentle ping 🏓

@dkotik
Copy link
Contributor Author

dkotik commented Sep 18, 2023

@dkotik gentle ping 🏓

Dear friend, I am not a maintainer here. I don't have the merge powers. What do I need to do?

@GreyXor
Copy link

GreyXor commented Sep 18, 2023

@dkotik gentle ping 🏓

Dear friend, I am not a maintainer here. I don't have the merge powers. What do I need to do?

@dkotik Greetings! I'm not requesting a merge at this time; rather, I'm reaching out for a code review. As you can observe, I've provided four suggestions for enhancing your code. Could you please take a moment to review them?

@dkotik
Copy link
Contributor Author

dkotik commented Sep 19, 2023

@dkotik gentle ping 🏓

Dear friend, I am not a maintainer here. I don't have the merge powers. What do I need to do?

@dkotik Greetings! I'm not requesting a merge at this time; rather, I'm reaching out for a code review. As you can observe, I've provided four suggestions for enhancing your code. Could you please take a moment to review them?

I am confused. I am a Github newb, or there are something with permissions - I do not see any review comments in the code itself or in the list of "Reviewers" to the top right of this.

slog.go Outdated Show resolved Hide resolved
slog.go Outdated Show resolved Hide resolved
slog.go Outdated Show resolved Hide resolved
slog.go Outdated Show resolved Hide resolved
@GreyXor
Copy link

GreyXor commented Sep 19, 2023

Thanks for your input, now you should be able to see it.

@dkotik dkotik requested a review from GreyXor September 19, 2023 16:52
@GreyXor
Copy link

GreyXor commented Sep 19, 2023

LGTM, very good thanks!
@m110 can we consider a merge?

@m110
Copy link
Member

m110 commented Sep 24, 2023

@dkotik can you run go mod tidy on the repository, please? :)

slog.go Show resolved Hide resolved
result := make([]any, 0, len(fields)*2)

for key, value := range fields {
// result = append(result, slog.Any(key, value))
Copy link
Member

Choose a reason for hiding this comment

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

Can we now drop the comment?

@roblaszczak
Copy link
Member

Hey @dkotik, LGTM! Thanks for the contribution!

I'll address #355 (comment) and fix CI on the main branch.

@roblaszczak roblaszczak merged commit a9a238f into ThreeDotsLabs:master Sep 24, 2023
5 of 7 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.

4 participants