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

Use slog in place of waterlog #14

Merged
merged 1 commit into from
Sep 6, 2023
Merged

Use slog in place of waterlog #14

merged 1 commit into from
Sep 6, 2023

Conversation

livingsilver94
Copy link
Member

slog is the new structured logging library from the Go project itself. It is available as an experimental package for Go 1.20 and was upstreamed in Go 1.21. This makes it a perfect candidate for our tooling, because:

  • it won't bit-rot
  • we can use a human-appealing Handler for CLI, and e.g. a JSON Handler on our builders, without changing the code

This PR only replaces waterlog with slog, using its TextHandler Handler. TextHandler is nowhere near the look of waterlog, but a custom Handler mimicking the old look is in the works and will come with a different PR in the near future.

@livingsilver94
Copy link
Member Author

@silkeh would you mind rebasing #13 after this PR is accepted so that we don't keep a miles-long backlog? :) Ideally I would like to see usysconf back amongst the living in two weeks.

EbonJaeger
EbonJaeger previously approved these changes Sep 4, 2023
Copy link
Member

@EbonJaeger EbonJaeger left a comment

Choose a reason for hiding this comment

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

LGTM, though the changes in config.load.go look like a bit more than just changing out the logger. Change scope is something to watch out for in the future, because it makes it harder to review and harder to debug later to figure out what changed caused an issue.

@livingsilver94
Copy link
Member Author

I agree with you, but the code was way more duplicated than I could tolerate 😬 I can split the commit anyway.

Copy link
Member

@silkeh silkeh left a comment

Choose a reason for hiding this comment

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

Great work! I have a few relatively minor comments, but it looks fine otherwise 🙂

cli/list.go Outdated Show resolved Hide resolved
cli/list.go Outdated Show resolved Hide resolved
config/load.go Outdated Show resolved Hide resolved
config/load.go Outdated Show resolved Hide resolved
state/map.go Outdated Show resolved Hide resolved
state/map.go Outdated Show resolved Hide resolved
@livingsilver94
Copy link
Member Author

All sorted, I also squashed the commits.

Copy link
Member

@silkeh silkeh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@silkeh silkeh merged commit 121c8a9 into master Sep 6, 2023
@livingsilver94 livingsilver94 deleted the slog branch September 6, 2023 09:56
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