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

replace logrus with log/slog from stdlib #55

Merged
merged 15 commits into from
Nov 14, 2023
Merged

Conversation

perbu
Copy link
Collaborator

@perbu perbu commented Nov 6, 2023

The default now is not to log anything.
So, if the application needs logging, it wouldn't do this:

	engine := actor.NewEngine()

but rather

	engine := actor.NewEngine(actor.Config{Logger: log.Default()})

for the default logging (which can be changed)
or for more control:

	lh := log.NewHandler(os.Stdout, log.JsonFormat, slog.LevelDebug)
	engine := actor.NewEngine(actor.Config{Logger: log.NewLogger("[engine]", lh)})

Since slog is so versatile this should be good enough for anybody.

No big semantic changes. I did remove the log.Fatal and log.Panic-level and replaces these with panics.
Removes uses of the log package from the examples, they shouldn't be using the library for logging, but rather log directly with log or slog.

Note that one test is failing, it was failing before as well.

Overall, I like the package. God job.

Closes: #54

@anthdm
Copy link
Owner

anthdm commented Nov 13, 2023

@perbu Looking into this right now. Thanks for this upfront!

Copy link
Owner

@anthdm anthdm left a comment

Choose a reason for hiding this comment

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

Thank you for this amazing PR. Just one minor thing that could be very helpful, is to add some documentation in the README about on how to use the logger maybe?

Copy link
Collaborator Author

@perbu perbu left a comment

Choose a reason for hiding this comment

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

requested changes done.

@perbu
Copy link
Collaborator Author

perbu commented Nov 13, 2023

Thank you for this amazing PR. Just one minor thing that could be very helpful, is to add some documentation in the README about on how to use the logger maybe?

Hmm. How do I tell github that I did what you asked? 😕

@perbu perbu requested a review from anthdm November 13, 2023 10:14
@anthdm
Copy link
Owner

anthdm commented Nov 13, 2023

Thank you for this amazing PR. Just one minor thing that could be very helpful, is to add some documentation in the README about on how to use the logger maybe?

Hmm. How do I tell github that I did what you asked? 😕

IDK :)

@anthdm
Copy link
Owner

anthdm commented Nov 13, 2023

For some reason its not willing to pass the build system, due to some slog package shenanigans. Would be cool if you could look into that.

@perbu
Copy link
Collaborator Author

perbu commented Nov 13, 2023

@anthdm I don't see any problems with the CI anymore. I've bumped the GH action to use 1.20, that was the ticket.

@anthdm anthdm merged commit 58f1195 into anthdm:master Nov 14, 2023
1 check 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.

replace logrus with slog?
2 participants