Skip to content
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

refactor: remove odp config from constructors #406

Merged

Conversation

andrewleap-optimizely
Copy link
Contributor

Summary

  • remove odp_config from contructors for odp event manager and odp segment manager

Test plan

all tests pass

Ticket

@andrewleap-optimizely andrewleap-optimizely changed the title fix: remove odp config from constructors refactor: remove odp config from constructors Sep 26, 2022
@andrewleap-optimizely andrewleap-optimizely marked this pull request as ready for review September 26, 2022 20:52
@andrewleap-optimizely andrewleap-optimizely requested a review from a team as a code owner September 26, 2022 20:52
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

@@ -217,6 +219,10 @@ def stop(self) -> None:

def send_event(self, type: str, action: str, identifiers: dict[str, str], data: OdpDataDict) -> None:
"""Create OdpEvent and add it to the event queue."""
if not self.odp_config:
self.logger.debug('ODP event queue: cannot send before config has been set.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to have a test/ assertion for this new log?

Copy link
Contributor

@Mat001 Mat001 left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewleap-optimizely andrewleap-optimizely merged commit 92ab102 into master Sep 27, 2022
@andrewleap-optimizely andrewleap-optimizely deleted the aleap/remove_odp_config_from_constructors branch September 27, 2022 13:12
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.

None yet

3 participants