Skip to content

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

Merged
merged 2 commits into from
Feb 8, 2023

Conversation

yasirfolio3
Copy link
Contributor

@yasirfolio3 yasirfolio3 commented Feb 2, 2023

Summary

  • Provides access to some ODP classes for testing purpose.

Test plan

Issues

  • FSSDK-8042

@coveralls
Copy link

coveralls commented Feb 2, 2023

Coverage Status

Coverage: 95.245% (-0.1%) from 95.355% when pulling 54dad12 on yasir/fsc-odp-compat-fix into a374eed on master.

Copy link
Contributor

@jaeopt jaeopt left a 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.

@@ -16,7 +16,7 @@

import Foundation

class OdpConfig {
public class OdpConfig {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason to open public?

Copy link
Contributor Author

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.

Copy link
Contributor

@jaeopt jaeopt Feb 2, 2023

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

@yasirfolio3
Copy link
Contributor Author

yasirfolio3 commented Feb 8, 2023

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@msohailhussain msohailhussain merged commit 3fce84a into master Feb 8, 2023
@msohailhussain msohailhussain deleted the yasir/fsc-odp-compat-fix branch February 8, 2023 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants