feat: events #357
feat: events #357
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
| @property | ||
| def features(self) -> dict: | ||
| if not hasattr(self, "_parsed_payload"): | ||
| self._parsed_payload = loads(self.raw_features)["features"] |
There was a problem hiding this comment.
Annoyingly this now becomes part of the public API.
Kinda struggled a little with choosing between only returning the features vs returning the whole response. Leaning ever so slightly in the direction of only returning the features since its less confusing for end users.
That being said, every time we've done that in the past (fetch features, bootstrap, load from backup), it's been a mistake that needs to be patched later. Open to being told this is similar but I can't see how right now
There was a problem hiding this comment.
This is still a dict, I think it'd be different if this is an array because it's not easy to extend an array with other properties, but being a dict I think this gives enough flexibility. And I agree, returning less is better IMO
| """ | ||
|
|
||
| if not event_callback: | ||
| return None |
There was a problem hiding this comment.
Poor mans optional.map()
There was a problem hiding this comment.
I wonder if it makes sense to support this optional parameter. IMO, if you're going to call this function, you better have a callback. If you supply something that's not a callback then you should expect an error. But having this ability to call all functions with optional and checking if it's set seems a bit off.
On the other hand, if this function can be called from multiple places it centralizes the check in one single place in the code, so just picking your brain about it.
| raw_features: str | ||
|
|
||
| @property | ||
| def features(self) -> dict: |
There was a problem hiding this comment.
Parsing is JSON deserialize, which is expensive. This gives us lazy, cached parse here which the cost is only incurred on demand
gastonfournier
left a comment
gastonfournier
left a comment
There was a problem hiding this comment.
This looks nice, just left you a couple of stylish comments
| """ | ||
|
|
||
| if not event_callback: | ||
| return None |
There was a problem hiding this comment.
I wonder if it makes sense to support this optional parameter. IMO, if you're going to call this function, you better have a callback. If you supply something that's not a callback then you should expect an error. But having this ability to call all functions with optional and checking if it's set seems a bit off.
On the other hand, if this function can be called from multiple places it centralizes the check in one single place in the code, so just picking your brain about it.
| scheduler_executor: Optional[str] = None, | ||
| multiple_instance_mode: InstanceAllowType = InstanceAllowType.WARN, | ||
| event_callback: Optional[Callable[[UnleashEvent], None]] = None, | ||
| event_callback: Optional[Callable[[BaseEvent], None]] = None, |
There was a problem hiding this comment.
| event_callback: Optional[Callable[[BaseEvent], None]] = None, | |
| event_callback: Callable[[BaseEvent], None] = lambda event: None, |
Following my other comment, the alternative would be something like this.
| @property | ||
| def features(self) -> dict: | ||
| if not hasattr(self, "_parsed_payload"): | ||
| self._parsed_payload = loads(self.raw_features)["features"] |
There was a problem hiding this comment.
This is still a dict, I think it'd be different if this is an array because it's not easy to extend an array with other properties, but being a dict I think this gives enough flexibility. And I agree, returning less is better IMO
| if event_callback: | ||
| event = UnleashFetchedEvent( | ||
| event_type=UnleashEventType.FETCHED, | ||
| event_id=uuid.uuid4(), | ||
| raw_features=state, | ||
| ) | ||
| event_callback(event) | ||
| if ready_callback: | ||
| ready_callback() |
There was a problem hiding this comment.
I think I kind of like the idea of removing the if statement and having functions doing nothing.
3 participants
Adds in events for when features are fetched and when the SDK becomes ready.
This brings us more inline with the other SDKs which have these events at minimum.
Closes #356