feat(notification-center): Add LogEvent notification #204
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…into mnoman/log_event_notification # Conflicts: # tests/test_event_processor.py
mjc1283
approved these changes
Aug 22, 2019
tests/test_notification_center.py
Outdated
| @@ -219,6 +235,7 @@ def test_clear_all_notification_listeners(self): | |||
| test_notification_center.clear_all_notification_listeners() | |||
|
|
|||
| for notification_type in notification_center.NOTIFICATION_TYPES: | |||
| print(notification_type) | |||
…/python-sdk into mnoman/log_event_notification
…/python-sdk into mnoman/log_event_notification
optimizely/event/event_processor.py
Outdated
| @@ -23,7 +23,7 @@ | |||
| from optimizely import logger as _logging | |||
| from optimizely.closeable import Closeable | |||
| from optimizely.event_dispatcher import EventDispatcher as default_event_dispatcher | |||
| from optimizely.helpers import validator | |||
| from optimizely.helpers import validator, enums | |||
There was a problem hiding this comment.
nit. I'd recommend splitting up imports into separate lines. Just a conventional thing.
optimizely/event/event_processor.py
Outdated
| @@ -60,7 +61,8 @@ def __init__(self, | |||
| event_queue=None, | |||
| batch_size=None, | |||
| flush_interval=None, | |||
| timeout_interval=None): | |||
| timeout_interval=None, | |||
| notification_center=None): | |||
There was a problem hiding this comment.
Can you add some good explanations for all the fields here. Something like: https://github.com/optimizely/python-sdk/blob/master/optimizely/optimizely.py#L42-L59
aliabbasrizvi
suggested changes
Sep 12, 2019
|
this PR is getting failed on travis. Need to check. |
aliabbasrizvi
suggested changes
Sep 16, 2019
optimizely/event/event_processor.py
Outdated
| Args: | ||
| event_dispatcher: Provides a dispatch_event method which if given a URL and params sends a request to it. | ||
| logger: Provides a log method to log messages. By default nothing would be logged. | ||
| default_start: Optional boolean param which starts the consumer thread if set to True. |
There was a problem hiding this comment.
Not entirely sure on this variable name. How about start_on_init?
optimizely/event/event_processor.py
Outdated
| batch_size: Optional param which defines the upper limit of the number of events in event_queue after which | ||
| the event_queue will be flushed. | ||
| flush_interval: Optional param which defines the time in milliseconds after which event_queue will be flushed. | ||
| timeout_interval: Optional param which defines the time in milliseconds before joining the consumer |
There was a problem hiding this comment.
I'd like for us to have any time intervals in seconds like we already have for datafile management.
# Conflicts: # optimizely/event/event_processor.py # tests/test_event_processor.py
# Conflicts: # optimizely/event/event_processor.py
# Conflicts: # optimizely/event/event_processor.py
…/python-sdk into mnoman/log_event_notification
aliabbasrizvi
approved these changes
Sep 24, 2019
| @@ -195,6 +200,12 @@ def _flush_queue(self): | |||
|
|
|||
| log_event = EventFactory.create_log_event(to_process_batch, self.logger) | |||
|
|
|||
| if self.notification_center is not None: | |||
There was a problem hiding this comment.
Should we be validating that notification_center is an instance of notification_center.NotificationCenter or does that happen elsewhere?
tests/test_event_processor.py
Outdated
| @@ -371,3 +375,29 @@ def test_init__NaN_timeout_interval(self): | |||
| # default timeout interval is 5s. | |||
| self.assertEqual(self._event_processor.timeout_interval, timedelta(seconds=5)) | |||
| mock_config_logging.info.assert_called_with('Using default value for timeout_interval.') | |||
|
|
|||
| def test_notification_center(self): | |||
There was a problem hiding this comment.
nit. test_notification_center__on_log_event
aliabbasrizvi
added a commit
that referenced
this pull request
Oct 4, 2019
This reverts commit 7d291fe.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Test plan