Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce the StoreItem base class #1158

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gycsaba96
Copy link
Contributor

This is a followup to PR #1151 removing duplicated functionality. The main benefits include:

  • reduced code duplication,
  • isolated testing of common functionality,
  • the possibility of a more targeted testing of the BaseStore class (which I would be happy to do).

The main changes are as follows:

  • introduced a new class that implements common functionality for items in a BaseStore,
  • added corresponding unit tests,
  • fixed a typo in the copyright text.

- introduced a new class that implements common functionality for
  items in a BaseStore
- added corresponding unit tests
- fixed a typo in the copyright text
Copy link
Contributor

@SqAtx SqAtx left a comment

Choose a reason for hiding this comment

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

I think it's a good improvement! Just some minor comments.

def __init__(self,id: UUID):
self.id: UUID = id
self.parent: Optional[Self] = None
self.children: List[Self] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. (We should also update the readme and every other reference to python version.)

self.id: UUID = id
self.parent: Optional[Self] = None
self.children: List[Self] = []
super(StoreItem, self).__init__()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
super(StoreItem, self).__init__()
super().__init__()

Bad habits form Python 2 still in the code base :) Let's fix them as we see them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return ancestors


def check_possible_parent(self, target) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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):?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

self.assertFalse(item.has_children)


def test_children_count_singel_child(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def test_children_count_singel_child(self):
def test_children_count_single_child(self):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

- 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants