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

Add deflection to custom __lt__ and add functools.total_ordering #447

Merged
merged 18 commits into from
Dec 19, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 13 additions & 38 deletions aea/configurations/data_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
SERVICE,
SKILL,
)
from aea.exceptions import enforce
from aea.helpers.base import (
IPFSHash,
IPFSHashOrStr,
Expand Down Expand Up @@ -122,13 +121,8 @@ def __eq__(self, other: Any) -> bool:

def __lt__(self, other: Any) -> bool:
"""Compare with another object."""
enforce(
isinstance(other, PackageVersion),
f"Cannot compare {type(self)} with type {type(other)}.",
)
other = cast(PackageVersion, other)
if self.is_latest or other.is_latest:
return self.is_latest < other.is_latest
if not isinstance(other, self.__class__):
return NotImplemented # Delegate comparison to the other instance.
return str(self) < str(other)


Expand Down Expand Up @@ -215,6 +209,7 @@ def __str__(self) -> str:
PackageIdPrefix = Tuple[ComponentType, str, str]


@functools.total_ordering
class PublicId(JSONSerializable):
"""This class implement a public identifier.

Expand Down Expand Up @@ -461,37 +456,14 @@ def __eq__(self, other: Any) -> bool:
return all(getattr(self, s) == getattr(other, s) for s in self.__slots__[:-1])

def __lt__(self, other: Any) -> bool:
"""
Compare two public ids.

>>> public_id_1 = PublicId("author_1", "name_1", "0.1.0")
>>> public_id_2 = PublicId("author_1", "name_1", "0.1.1")
>>> public_id_3 = PublicId("author_1", "name_2", "0.1.0")
>>> public_id_1 > public_id_2
False
>>> public_id_1 < public_id_2
True

>>> public_id_1 < public_id_3
Traceback (most recent call last):
...
ValueError: The public IDs author_1/name_1:0.1.0 and author_1/name_2:0.1.0 cannot be compared. Their author or name attributes are different.

:param other: the object to compate to
:raises ValueError: if the public ids cannot be confirmed
:return: whether or not the inequality is satisfied
"""
if (
isinstance(other, PublicId)
and self.author == other.author
and self.name == other.name
):
return self.package_version < other.package_version
raise ValueError(
"The public IDs {} and {} cannot be compared. Their author or name attributes are different.".format(
self, other
"""Compare with another object."""

Choose a reason for hiding this comment

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

The old docstring was nice?

Copy link
Author

Choose a reason for hiding this comment

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

bloat imo

if not isinstance(other, self.__class__):
return NotImplemented # Delegate comparison to the other instance.
if not (self.author == other.author and self.name == other.name):
raise TypeError(
f"The public IDs {self} and {other} cannot be compared. Their author or name attributes are different."
)
)
return self.package_version < other.package_version

def without_hash(
self,
Expand All @@ -518,6 +490,7 @@ def __str__(self) -> str:
)


@functools.total_ordering
class PackageId:
"""A package identifier."""

Expand Down Expand Up @@ -655,6 +628,8 @@ def __eq__(self, other: Any) -> bool:

def __lt__(self, other: Any) -> bool:
"""Compare two public ids."""
if not isinstance(other, self.__class__):
return NotImplemented # Delegate comparison to the other instance.
return str(self) < str(other)


Expand Down
29 changes: 3 additions & 26 deletions docs/api/configurations/data_types.md
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ Get the string representation.
## PublicId Objects

```python
@functools.total_ordering
class PublicId(JSONSerializable)
```

Expand Down Expand Up @@ -531,32 +532,7 @@ Check equality.
def __lt__(other: Any) -> bool
```

Compare two public ids.

>>> public_id_1 = PublicId("author_1", "name_1", "0.1.0")
>>> public_id_2 = PublicId("author_1", "name_1", "0.1.1")
>>> public_id_3 = PublicId("author_1", "name_2", "0.1.0")
>>> public_id_1 > public_id_2
False
>>> public_id_1 < public_id_2
True

>>> public_id_1 < public_id_3
Traceback (most recent call last):
...
ValueError: The public IDs author_1/name_1:0.1.0 and author_1/name_2:0.1.0 cannot be compared. Their author or name attributes are different.

**Arguments**:

- `other`: the object to compate to

**Raises**:

- `ValueError`: if the public ids cannot be confirmed

**Returns**:

whether or not the inequality is satisfied
Compare with another object.

<a id="aea.configurations.data_types.PublicId.without_hash"></a>

Expand Down Expand Up @@ -593,6 +569,7 @@ Get the string representation.
## PackageId Objects

```python
@functools.total_ordering
class PackageId()
```

Expand Down
8 changes: 8 additions & 0 deletions tests/strategies/data_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
GitRef,
PackageId,
PackageType,
PackageVersion,
PublicId,
PyPIPackageName,
)
Expand Down Expand Up @@ -70,6 +71,13 @@
st.register_type_strategy(semver.VersionInfo, version_info_strategy)


package_version_strategy = st.builds(
lambda kwargs: PackageVersion(**kwargs),
st.fixed_dictionaries(dict(version_like=version_info_strategy)),
)
st.register_type_strategy(PackageVersion, package_version_strategy)


s = st.fixed_dictionaries(
dict(
author=st.from_type(SimpleId),
Expand Down
4 changes: 2 additions & 2 deletions tests/test_configurations/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -707,7 +707,7 @@ def test_public_id_comparator_when_author_is_different():
pid1 = PublicId("author_1", "name", "0.1.0")
pid2 = PublicId("author_2", "name", "0.1.0")
with pytest.raises(
ValueError,
TypeError,
match="The public IDs .* and .* cannot be compared. Their author or name attributes are different.",
):
assert pid1 < pid2
Expand All @@ -718,7 +718,7 @@ def test_public_id_comparator_when_name_is_different():
pid1 = PublicId("author", "name_1", "0.1.0")
pid2 = PublicId("author", "name_2", "0.1.0")
with pytest.raises(
ValueError,
TypeError,
match="The public IDs .* and .* cannot be compared. Their author or name attributes are different.",
):
assert pid1 < pid2
Expand Down
6 changes: 2 additions & 4 deletions tests/test_configurations/test_data_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,12 @@ def all_comparisons_operations_equal(pair_a, pair_b) -> bool:
return version_comparison == package_comparison


@pytest.mark.parametrize("self_type", [PublicId, PackageId, Dependency])
@pytest.mark.parametrize("self_type", [PackageVersion, PublicId, PackageId])
def test_self_type_comparison(self_type):
"""Test comparison to self"""

self = st.from_type(self_type).example()
copy_self = copy.deepcopy(self)
other = st.from_type(self_type).example()
assert self == copy_self
other = copy.deepcopy(self)
for f in COMPARISON_OPERATORS:
assert isinstance(f(self, other), bool)
assert isinstance(f(other, self), bool)
Comment on lines 71 to 75
Copy link
Author

Choose a reason for hiding this comment

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

we should copy, we don't want to test raising a TypeError here when certain fields (e.g. author and or name on PublicId) are not identical. That is done in

@pytest.mark.parametrize("self_type", [PublicId, PackageId, Dependency])
@pytest.mark.parametrize(
"other_type", [*NUMERIC_TYPES, *ITERATOR_TYPES, PublicId, PackageId, Dependency]
)
def test_type_comparison(self_type, other_type):
"""Test type comparison"""
if self_type is other_type:
return
funcs = (operator.le, operator.lt, operator.ge, operator.gt)
self = st.from_type(self_type).example()
other = st.from_type(other_type).example()
assert not isinstance(self, other_type)
assert not isinstance(other, self_type)
assert self.__eq__(self)
assert other.__eq__(other)
assert self.__ne__(other)
assert other.__ne__(self)
for f in funcs:
with pytest.raises((TypeError, ValueError)):
assert f(self, other)

Expand Down