open-feature / go-sdk

Go SDK for OpenFeature
https://openfeature.dev
Apache License 2.0
124 stars 29 forks source link

[FEATURE] Review and simplify application author facing APIs if possible #62

Closed skyerus closed 1 year ago

skyerus commented 1 year ago

Requirements

While other breaking changes are taking place (https://github.com/open-feature/golang-sdk/issues/56 & https://github.com/open-feature/golang-sdk/issues/57) it's a good time to review whether any quality of life improvements can be made to application author facing APIs.

skyerus commented 1 year ago

I've opened PRs for two potential improvements to the client API, we need to decide which approach is preferable (I prefer the second), or perhaps some combination of the two.

  1. https://github.com/open-feature/golang-sdk/pull/63 This separates the flag calls into two, the first is a simplified call without the EvaluationOptions parameter, the second is as before. This retains the enforcement of including the EvaluationContext in all flag resolution calls.

  2. https://github.com/open-feature/golang-sdk/pull/64 This also separates the flag calls into two, both of which now have EvaluationOptions as a variadic parameter in order to simulate an "optional" parameter. The original is now a simplified call without the EvaluationContext parameter with the addition of a new WithContext func that includes the EvaluationContext parameter.

@james-milligan @beeme1mr @toddbaert

james-milligan commented 1 year ago

I prefer the variadic approach, it makes for a cleaner interface

skyerus commented 1 year ago

@toddbaert does your approval of https://github.com/open-feature/golang-sdk/pull/63 mean that's your preferred approach? James & I prefer the other approach

skyerus commented 1 year ago

We settled on a middle ground approach of retaining the EvaluationContext as a first class parameter and making the EvaluationOptions variadic

victorkt commented 1 year ago

Apologies if this was already discussed and I realise I might be a bit late to this, but have you considered using functional options? It's an idiomatic and common way of passing options in Go.

As an example, I can point to OpenTelemetry's implementation for instrumentation options.

For OpenFeature, it could look something like this:

val, err := client.BooleanValue(
  "someFlag",
  false,
  openfeature.EvaluationContext{},
  openfeature.WithHooks(...),
  openfeature.WithHints(...),
)

All options would be optional, and It achieves something similar to what is being proposed here. One drawback of the current proposed approach would have to deal with multiple EvaluationOptions objects being passed (either ignoring or merging them).

skyerus commented 1 year ago

Thanks for the eloquent explanation and example. While I haven't seen this approach used in function calls (only seen in constructors e.g. func NewFoo()), I don't see why we couldn't. I actually think this is cleaner than our current variadic EvaluationOptions, I'm all for it. Interested to hear some other opinions on the matter though @james-milligan @toddbaert @beeme1mr @davejohnston

toddbaert commented 1 year ago

@victorkt @skyerus ... what's not to like about this option?! Makes sense to me. I give it a +1.

Thanks for the suggestion.