Updates __eq__ implementation on the data types
#445
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Karrenbelt
commented
Nov 16, 2022
Comment on lines
+89
to
+90
| __slots__ = ("_version",) | ||
|
|
There was a problem hiding this comment.
such that a generic solution could be implemented that is equal for all classes in this module
Karrenbelt
commented
Nov 16, 2022
Karrenbelt
commented
Nov 16, 2022
Comment on lines
650
to
660
| def __eq__(self, other: Any) -> bool: | ||
| """Compare with another object.""" | ||
| return ( | ||
| isinstance(other, PackageId) | ||
| and self.package_type == other.package_type | ||
| and self.public_id == other.public_id | ||
| ) | ||
| """Check equality.""" | ||
| if not isinstance(other, self.__class__): | ||
| return NotImplemented # Delegate comparison to the other instance's __eq__. | ||
| return all(getattr(self, s) == getattr(other, s) for s in self.__slots__) | ||
|
|
There was a problem hiding this comment.
open-aea/aea/configurations/data_types.py
Line 536 in 8e3ce14
Karrenbelt
commented
Nov 16, 2022
Comment on lines
875
to
890
| def __eq__(self, other: Any) -> bool: | ||
| """Compare with another object.""" | ||
| return ( | ||
| isinstance(other, Dependency) | ||
| and self._name == other._name | ||
| and self._version == other._version | ||
| and self._index == other._index | ||
| and self._git == other._git | ||
| and self._ref == other._ref | ||
| ) | ||
| """Check equality.""" | ||
| if not isinstance(other, self.__class__): | ||
| return NotImplemented # Delegate comparison to the other instance's __eq__. | ||
| return all(getattr(self, s) == getattr(other, s) for s in self.__slots__) | ||
|
|
There was a problem hiding this comment.
open-aea/aea/configurations/data_types.py
Line 764 in 8e3ce14
8e3ce14 to
7cd21b5
Compare
Karrenbelt
commented
Nov 16, 2022
Karrenbelt
commented
Nov 17, 2022
Comment on lines
+458
to
+461
| """Check equality.""" | ||
| if not isinstance(other, self.__class__): | ||
| return NotImplemented # Delegate comparison to the other instance's __eq__. | ||
| return all(getattr(self, s) == getattr(other, s) for s in self.__slots__[:-1]) |
There was a problem hiding this comment.
including the _package_hash will affect the functionality in the following 4 places, which is why I chose not to include it now (it was not included in the original comparison either). We may choose to open an issue on this if we want to change this later.
eject
Lines 204 to 206 in 77474ff
remove
Lines 193 to 226 in 77474ff
check_packages
open-aea/aea/cli/check_packages.py
Lines 309 to 310 in 77474ff
registry/add
open-aea/aea/cli/registry/add.py
Lines 31 to 72 in 77474ff
There was a problem hiding this comment.
Please open an issue for this and add a comment in code.
|
No longer failing the following (was so after 3b27e78 - see CI run) all remaining failures are those related to newly introduced tests in base PR #428 |
__eq____eq__ implementation on the data types
__eq__ implementation on the data types__eq__ implementation on the data types
__eq__ implementation on the data types__eq__ implementation on the data types
08025c6 to
24d3fd1
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## tests/test_data_types #445 +/- ##
========================================================
Coverage ? 97.31%
========================================================
Files ? 221
Lines ? 19540
Branches ? 0
========================================================
Hits ? 19016
Misses ? 524
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
0xArdi
approved these changes
Dec 18, 2022
10 tasks
…nLike Fix/delegate `__lt__` to `PackageVersionLike`
Add deflection to custom `__lt__` and add `functools.total_ordering`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
Updating the implementation of custom
__eq__methods on custom data types:PackageVersion,PublicId,PackageIdandDependency.as per base PR description: #428
and for reference: googleapis/google-cloud-python#3455
Fixes
If it fixes a bug or resolves a feature request, be sure to link to that issue.
Types of changes
What types of changes does your code introduce to agents-aea?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply.developbranch (left side). Also you should start your branch off ourdevelop.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...