Introduce the StoreItem base class #1158
Conversation
- introduced a new class that implements common functionality for items in a BaseStore - added corresponding unit tests - fixed a typo in the copyright text
GTG/core/base_store.py
Outdated
| def __init__(self,id: UUID): | ||
| self.id: UUID = id | ||
| self.parent: Optional[Self] = None | ||
| self.children: List[Self] = [] |
There was a problem hiding this comment.
| self.children: List[Self] = [] | |
| self.children: list[Self] = [] |
Now that Python 3.8 is gone, we can write code that targets 3.9, which means we no longer need typing.List and the like.
There was a problem hiding this comment.
Done. (We should also update the readme and every other reference to python version.)
GTG/core/base_store.py
Outdated
| self.id: UUID = id | ||
| self.parent: Optional[Self] = None | ||
| self.children: List[Self] = [] | ||
| super(StoreItem, self).__init__() |
There was a problem hiding this comment.
| super(StoreItem, self).__init__() | |
| super().__init__() |
Bad habits form Python 2 still in the code base :) Let's fix them as we see them.
GTG/core/base_store.py
Outdated
| return ancestors | ||
|
|
||
|
|
||
| def check_possible_parent(self, target) -> bool: |
There was a problem hiding this comment.
| def check_possible_parent(self, target) -> bool: | |
| def is_parent_of(self, target) -> bool: |
and inverse the conditions below.
I think it might be clearer to read if not foo.is_parent_of(bar): rather than if foo.check_possible_parent(bar):?
There was a problem hiding this comment.
I agree that the name is not perfect. However, is_parent_of wouldn't correctly describe the method (e.g., you can not parent an item to itself.) I used is_parentable_to in the new version. I think it also reads much better than the original one.
tests/core/test_store_item.py
Outdated
| self.assertFalse(item.has_children) | ||
|
|
||
|
|
||
| def test_children_count_singel_child(self): |
There was a problem hiding this comment.
| def test_children_count_singel_child(self): | |
| def test_children_count_single_child(self): |
- use Python 3.9 type hint - simplify super() calls - fix a typo in a test case - rename check_possible_parent to is_parentabel_to Co-authored-by: Kevin Guillaumond <[email protected]>
diegogangl
added
enhancement
maintainability
diegogangl
added this to the 0.7: "Getting things Gradius — Destroy the core!" milestone
GTG/core/base_store.py
Outdated
| return len(self.children) > 0 | ||
|
|
||
|
|
||
| def get_ancestors(self) -> List[Self]: |
There was a problem hiding this comment.
If we are moving away from typing.List, this can also be replaced with list
GTG/core/base_store.py
Outdated
|
|
||
| def get_ancestors(self) -> List[Self]: | ||
| """Return all ancestors of this tag""" | ||
| ancestors: List[Self] = [] |
All replaced now in the |
|
LGTM now, thanks! |
This is a followup to PR #1151 removing duplicated functionality. The main benefits include:
BaseStoreclass (which I would be happy to do).The main changes are as follows:
BaseStore,