feat: implement `_snapshot_client` for `Snapshotter` by Mantisus · Pull Request #957 · apify/crawlee-python

feat: implement `_snapshot_client` for `Snapshotter` by Mantisus · Pull Request #957 · apify/crawlee-python

This comment was marked as outdated.

		client = service_locator.get_storage_client()

		error_count = 0
		rate_limit_errors: dict[int, int] = getattr(client, 'rate_limit_errors', {})

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

I have some concerns/questions.

Choose a reason for hiding this comment

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

Thanks for the explanation.

Choose a reason for hiding this comment

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

Apart from that one comment, I'm also not sure that this passes as a chore: 🙂

		client = service_locator.get_storage_client()

		error_count = 0
		rate_limit_errors: dict[int, int] = getattr(client, 'rate_limit_errors', {})

Choose a reason for hiding this comment

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

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.

Apart from that one comment, I'm also not sure that this passes as a chore: 🙂

I'm not sure about the chore type either.

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....

Apart from that one comment, I'm also not sure that this passes as a chore: 🙂

I'm not sure about the chore type either.

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 feat 🙂 Let's edit that and merge!

Mantisus changed the title ~~chore: implement _snapshot_client for Snapshotter~~ feat: implement _snapshot_client for Snapshotter