-
Notifications
You must be signed in to change notification settings - Fork 32
refact(odp): Allows support for ODP testing using FSC. #476
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great. One change suggested - I do not see the reason we want to open OdpConfig.
Sources/ODP/OdpConfig.swift
Outdated
@@ -16,7 +16,7 @@ | |||
|
|||
import Foundation | |||
|
|||
class OdpConfig { | |||
public class OdpConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason to open public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary since OdpEventManager
's contructor requires this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! I removed OdpConfig from OdpEventManager and OdpSegmentManager init methods to support customization without opening OdpConfig. Please review this PR first (#476) and then we can remove public
here. @yasirfolio3
@jaeopt @msohailhussain Verified it with latest FSC tests: https://github.com/optimizely/fullstack-sdk-compatibility-suite/actions/runs/4122123976/jobs/7118530491 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
Test plan
Issues