-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adding eq method to collection #41
base: pipeline
Are you sure you want to change the base?
Adding eq method to collection #41
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks fine (some suggestions for streamlining it), but the tests are failing.
I was trying to add a test too but it seems there was a problem with the test.
It should have been correct if the code for the test was running the new version, but that was not the case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests still needed.
|
||
def __eq__(self, other: Node) -> bool: | ||
|
||
for property in self.properties: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also need to check equality self._type_
and self.id
(if it is defined, i.e., for LinkedMetadata
, but not for EmbeddedMetadata
, and if it is not None)
def __eq__(self, other): | ||
|
||
# The current implementation assumes that nodes in both graphs are connected with the same link number. | ||
for node_id in self.nodes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also need to check that there are no extra nodes in other
that are not present in self
.
@@ -34,6 +34,18 @@ def __len__(self): | |||
|
|||
def __iter__(self): | |||
return iter(self.nodes.values()) | |||
|
|||
def __eq__(self, other): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a preliminary check that the len()
of both collections are the same? If they're different, we can return False
quickly without needing to check all the nodes.
No description provided.