feat(api): Add top-level wrapper for more convenient usage. #191
Conversation
mikeproeng37
requested review from
mikecdavis,
mjc1283,
pawels-optimizely and
thomaszurkan-optimizely
pawels-optimizely
left a comment
pawels-optimizely
left a comment
There was a problem hiding this comment.
I understand the concept, (currently, 2 main.go files don't run with these changes)
mikecdavis
left a comment
mikecdavis
left a comment
There was a problem hiding this comment.
I think this is a good start. I like having a clear entry point into the SDK. What are your thoughts on moving the entire contents of the client package into the root? So it's not a "wrapper" it just "is".
main.go
Outdated
| ***************************************************************************/ | ||
|
|
||
| package main | ||
| package optimizely |
There was a problem hiding this comment.
Looks like we can't mix the "application" (e.g. "package main") with an importable package. Perhaps we just remove this CLI and move it into the sidedoor repo instead?
There was a problem hiding this comment.
Yeah I think that would make it simpler.
examples/main.go
Outdated
|
|
||
| "github.com/optimizely/go-sdk" | ||
| "github.com/optimizely/go-sdk/pkg/client" | ||
| "github.com/optimizely/go-sdk/pkg/entities" |
There was a problem hiding this comment.
This import is no longer used.
There was a problem hiding this comment.
This is tricky because when imported, we import the namespace "optimizely", which is the package name and we would be referring to this import via optimizely.xxx
There was a problem hiding this comment.
I think @mikecdavis is talking about "github.com/optimizely/go-sdk/pkg/entities", this is unused.
| /************* StaticClient ********************/ | ||
|
|
||
| optimizelyClient, err := optimizelyFactory.StaticClient() | ||
| optimizelyClient, err = optimizelyFactory.StaticClient() |
There was a problem hiding this comment.
err is no longer defined.
Yeah I thought about doing that, but I didn't want to introduce that big of a breaking change, even though we are technically still beta. I'd like to reserve that for a v2 so we can avoid breaking existing usage as well as documentation. Something else I am considering though is whether we want to introduce singleton usage in v1 or we reserve that for v2. It will depend on feedback we get from customers. But I can see this being a broader SDK-wide feature that we enable across most other SDKs. |
|
I think this makes sense as a first step. Agree with @mikeng13 's comments about breaking changes, v2, customer feedback, and singleton/instance container. |
pawels-optimizely
left a comment
pawels-optimizely
left a comment
There was a problem hiding this comment.
looks good, we can also remove cobra package from go.mod if we don't need that anymore.
|
|
||
| Each supported SDK API method is it's own [cobra](https://github.com/spf13/cobra) command and requires the | ||
| input of an `--sdkKey`. | ||
| ## Installation |
There was a problem hiding this comment.
This will be easier to review by looking at the full file
examples/main.go
Outdated
|
|
||
| "github.com/optimizely/go-sdk" | ||
| "github.com/optimizely/go-sdk/pkg/client" | ||
| "github.com/optimizely/go-sdk/pkg/entities" |
There was a problem hiding this comment.
I think @mikecdavis is talking about "github.com/optimizely/go-sdk/pkg/entities", this is unused.
| } | ||
| ) | ||
| optimizelyClient, err := optimizely.Client(sdkKey) | ||
| enabled, _ := optimizelyClient.IsFeatureEnabled("mutext_feat", user) |
There was a problem hiding this comment.
I'm getting an error below because we define enabled here.
optimizely-codereview-bot
requested review from
mikecdavis,
mjc1283 and
pawels-optimizely
Summary