feat: implement _snapshot_client for Snapshotter
#957
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| client = service_locator.get_storage_client() | ||
|
|
||
| error_count = 0 | ||
| rate_limit_errors: dict[int, int] = getattr(client, 'rate_limit_errors', {}) |
There was a problem hiding this comment.
I need some explanation here... What values can be in rate_limit_errors? What value is going to be there for the MemoryStorageClient (nothing?) and for the ApifyStorageClient? Thanks.
There was a problem hiding this comment.
For MemoryStorageClient it is empty dict. Since getattr will return the default value if the specified attribute does not exist
For ApifyStorageClient it is rate_limit_errors from https://github.com/apify/apify-client-python/blob/master/src/apify_client/_statistics.py#L15. Relate PR - apify/apify-sdk-python#387
There was a problem hiding this comment.
I came here from apify/apify-sdk-python#387. I'd much prefer to add another method to the storage client interface, along with a dummy implementation (return {}) on the memory storage client.
Then we can avoid the getattr here, which is pretty confusing and non-standard.
| client = service_locator.get_storage_client() | ||
|
|
||
| error_count = 0 | ||
| rate_limit_errors: dict[int, int] = getattr(client, 'rate_limit_errors', {}) |
There was a problem hiding this comment.
I came here from apify/apify-sdk-python#387. I'd much prefer to add another method to the storage client interface, along with a dummy implementation (return {}) on the memory storage client.
Then we can avoid the getattr here, which is pretty confusing and non-standard.
I'm not sure about the On the one side, it has no effect on clients outside of the Apify platform. On the other side, we now add a new method to the public interface.... |
I mean, we're still on 0.x, so I wouldn't think twice about making it a |
Mantisus
changed the title
_snapshot_client for Snapshotter_snapshot_client for Snapshotter
Description
_snapshot_clientforSnapshotterIssues
Snapshotter._snapshot_client()#60