Skip to content

Conversation

jirimoravcik
Copy link
Member

@jirimoravcik jirimoravcik commented Jan 4, 2023

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
Copy link
Member

@fnesveda fnesveda left a comment

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.

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

Copy link
Member Author

@jirimoravcik jirimoravcik Jan 16, 2023

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

@jirimoravcik jirimoravcik requested a review from fnesveda January 16, 2023 10:38
Copy link
Member

@fnesveda fnesveda left a comment

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