Skip to content

feat: Implement Dataset, KeyValueStore classes, create storage management logic - #21

Merged
jirimoravcik merged 25 commits into
masterfrom
feature/implement-dataset-rq-kvs-classes
Jan 16, 2023
Merged

feat: Implement Dataset, KeyValueStore classes, create storage management logic#21
jirimoravcik merged 25 commits into
masterfrom
feature/implement-dataset-rq-kvs-classes

Conversation

@jirimoravcik

@jirimoravcik jirimoravcik commented Jan 4, 2023

Copy link
Copy Markdown
Member

Skipped RequestQueue class as it's quite big and not that related to Actor class. Will be in next PR

@github-actions github-actions Bot added this to the 54th sprint - Platform team milestone Jan 4, 2023
@github-actions github-actions Bot added the t-core-services Issues with this label are in the ownership of the core services team. label Jan 4, 2023
@jirimoravcik jirimoravcik changed the title feat: Implement Dataset, KeyValueStore and RequeestQueue classes feat: Implement Dataset, KeyValueStore classes, create storage management logic Jan 11, 2023
@jirimoravcik
jirimoravcik marked this pull request as ready for review January 11, 2023 22:04
@jirimoravcik
jirimoravcik requested a review from fnesveda January 11, 2023 22:04

@fnesveda fnesveda left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wow, looks great! I just had a couple of smaller comments, but otherwise I think it is good to go.

Comment thread tests/unit/conftest.py
Comment thread src/apify/actor.py Outdated
Comment thread src/apify/storages/_utils.py Outdated
Comment thread src/apify/storages/_utils.py Outdated
self._cache = OrderedDict()
self._max_length = max_length

def get(self, key: str) -> Optional[T]: # TODO: Consider making this __getitem__ to be more Pythonic

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wow, that would be really cool, if this had __getitem__, __setitem__ and __delitem__, then it would basically support the Mapping protocol and would be usable as a dictionary.

@jirimoravcik jirimoravcik Jan 16, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Found a cool SO post about it https://stackoverflow.com/questions/19775685/how-to-correctly-implement-the-mapping-protocol-in-python
I'll look into it when implementing request queue, but it would be quite nice

also found this: https://gist.github.com/davesteele/44793cd0348f59f8fadd49d7799bd306
it directly inherits an OrderedDict

Comment thread src/apify/storages/storage_manager.py Outdated
@jirimoravcik
jirimoravcik requested a review from fnesveda January 16, 2023 10:38

@fnesveda fnesveda left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great!

@jirimoravcik
jirimoravcik merged commit d1b357c into master Jan 16, 2023
@jirimoravcik
jirimoravcik deleted the feature/implement-dataset-rq-kvs-classes branch January 16, 2023 11:06
@fnesveda fnesveda added the validated Issues that are resolved and their solutions fulfill the acceptance criteria. label Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-core-services Issues with this label are in the ownership of the core services team. validated Issues that are resolved and their solutions fulfill the acceptance criteria.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants