optimizely / csharp-sdk

.NET based C# SDK for Optimizely Feature Experimentation and Optimizely Full Stack (legacy)
https://docs.developers.optimizely.com/experimentation/v4.0.0-full-stack/docs/csharp-sdk
Apache License 2.0
19 stars 20 forks source link

[FSSDK-9090] Default instantiation of OdpManager for Optimizely constructors #347

Closed mikechu-optimizely closed 1 year ago

mikechu-optimizely commented 1 year ago

Summary

OptimizelyFactory provides a default which is desirable. The Optimizely constructors should follow this.

Test plan

Issues

jaeopt commented 1 year ago

I have a question. How are we going to disable the odp? Previously we assumed that if user is not passing odpManager then it will be considered as disabled. In my opinion if someone is using OptimizelyFactory to initialize Optimizely then odpManager should be enabled by default otherwise do not initialize it and consider it disabled. @jaeopt What do you suggest?

ODP will be enabled by default and I see it can be disabled with the ODPManager builder. It may be inconvenient but should be ok for most use cases since enabled ODP functions will be ignored until they integrates ODP.

mikechu-optimizely commented 1 year ago

@mnoman09 I updated the https://github.com/optimizely/csharp-testapp/pull/83

mikechu-optimizely commented 1 year ago

Is it ok to have this PR open @jaeopt given my change to the test app to support FSC?

jaeopt commented 1 year ago

Is it ok to have this PR open @jaeopt given my change to the test app to support FSC?

@mikechu-optimizely oops, did I close the PR? I was wondering my comment has been broken and not realized I closed it by mistake. Sorry :)

mikechu-optimizely commented 1 year ago

Sweet. Passing now. I just hope I satisifed @mnoman09 's question to get a LGTM

mikechu-optimizely commented 1 year ago

Hey @mnoman09, I think you might be out, but I'm going to go ahead and merge this change. DM me if you wanna talk more about it. I'm happy to open more PRs to get it right.