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

WIP: 3PH #533

Draft
wants to merge 53 commits into
base: main
Choose a base branch
from
Draft

WIP: 3PH #533

wants to merge 53 commits into from

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Jun 25, 2023

There is much more to do, but this is the beginning of 3PH.

@zenhack zenhack marked this pull request as draft June 25, 2023 22:41
Fails currently; the exception returned is 'failed' rather than
'unimplemented', which suggests that it may not be being forwarded
correctly. Need to investigate further.

This also factors out a bunch of common logic into a helper function.
This gets rid of a panic("TODO: ...").

The test still fails, but I know why.
That is, don't, and clean up.

For good measure, move the snapshot.Release() out of the critical
section, as I'm not sure this is valid.
...did I really not run the tests after this?
Still fails because of the obvious panic("TODO..."), but caught a couple
unrelated bugs.
The new interface:

- Has levels (debug, info, warn, error)
- Accepts arguments for structured logging

Per the comments, the interface is designed to be used with
*slog.Logger, though other implementations are possible (and we already
use one in test).
I'm going to punt this off until after the basic 3PH, so let's make sure
this is correct after that.
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.

1 participant