Skip to content

Use slog in place of waterlog#14

Merged
silkeh merged 1 commit into
masterfrom
slog
Sep 6, 2023
Merged

Use slog in place of waterlog#14
silkeh merged 1 commit into
masterfrom
slog

Conversation

@livingsilver94
Copy link
Copy Markdown
Contributor

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

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

Copy link
Copy Markdown
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 🙂

Comment thread cli/list.go Outdated
Comment thread cli/list.go Outdated
Comment thread config/load.go Outdated
Comment thread config/load.go Outdated
Comment thread state/map.go Outdated
Comment thread state/map.go Outdated
@livingsilver94
Copy link
Copy Markdown
Contributor Author

All sorted, I also squashed the commits.

Copy link
Copy Markdown
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