feat: Add support for hooks. #126
Conversation
| stage ldmigration.Stage, | ||
| ) (ldmigration.Stage, interfaces.LDMigrationOpTracker, error) | ||
|
|
||
| func TestMigrationVariation(t *testing.T) { |
There was a problem hiding this comment.
Re-worked these tests to be parameterized so they could be ran against MigrationVariation and MigrationVariationEx.
ldclient.go
Outdated
| "github.com/launchdarkly/go-server-sdk/v7/subsystems" | ||
| "github.com/launchdarkly/go-server-sdk/v7/subsystems/ldstoreimpl" | ||
| ) | ||
|
|
||
| // Version is the SDK version. | ||
| const Version = internal.SDKVersion | ||
|
|
||
| const boolVarFuncName = "LDClient.BoolVariation" |
There was a problem hiding this comment.
Can surround this with a giant const block
ldclient.go
Outdated
| // has no off variation. | ||
| // | ||
| // For more information, see the Reference Guide: https://docs.launchdarkly.com/sdk/features/evaluating#go | ||
| func (client *LDClient) BoolVariationEx( |
There was a problem hiding this comment.
If the Ex methods are only differentiated by taking a Context, would you consider BoolVariationCtx instead as the convention?
There was a problem hiding this comment.
I considered Context and Ctx which I think make sense if your original function didn't also take a thing called Context. When I started calling them it seemed a little strange.
We could also add functional options right now as well, which would make them different, but I cannot think of any options to actually add. So they would be placeholder.
I could change to Ctx if you prefer.
There was a problem hiding this comment.
To my Go-brain, Ctx seems intuitive and I would prefer it. If I was unfamiliar with the LD concept of context I'd immediately know this took a context.Context.
There was a problem hiding this comment.
Side note: I think it'd be good to explain what the context is used for here. Like, the programmer might think "Ok, this takes a context, it must be a long running operation. Therefore if I want to end evaluation early, I should call context.Cancel()`.
There was a problem hiding this comment.
I did make a try at this, then I wondered if it would be a problem. If we decide we want to use the context for something, will it become a problem.
Maybe it wouldn't be a problem to specifically say that cancelling a context does not cancel an evaluation. Versus saying specifically what we use it for.
Co-authored-by: Casey Waldren <cwaldren@launchdarkly.com>
…go-server-sdk into rlamb/implement-hook-types
Co-authored-by: Casey Waldren <cwaldren@launchdarkly.com>
…o-server-sdk into rlamb/add-hooks-to-client
Second PR. This connects hooks to the ldclient. It does not add the `Ex` methods to any interface, and it does not add `AddHooks` to any interface. --------- Co-authored-by: Casey Waldren <cwaldren@launchdarkly.com>
🤖 I have created a release *beep* *boop* --- ## [7.3.0](v7.2.0...v7.3.0) (2024-04-03) This release introduces a Hooks API. Hooks are collections of user-defined callbacks that are executed by the SDK at various points of interest. You can use them to augment the SDK with metrics or tracing. ### Features * Add support for hooks. ([#126](#126)) ([929ef8b](929ef8b)) * Implement supporting types for hooks. ([#125](#125)) ([7f06147](7f06147)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
Second PR.
This connects hooks to the ldclient.
It does not add the
Exmethods to any interface, and it does not addAddHooksto any interface.