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

Conversation

Karrenbelt
Copy link

@Karrenbelt Karrenbelt commented Nov 17, 2022

Proposed changes

Adding deflection and functools.total_ordering to custom data types with a __lt__ implementation. PackageVersion, PublicId and PackageId.

Note that string casting is still using in __lt__ comparison; in that sense this PR is step 1 out of 2.

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 x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING doc
  • I am making a pull request against the develop branch (left side). Also you should start your branch off our develop.
  • Lint and unit tests pass locally with my changes and CI passes too
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that code coverage does not decrease.
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

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

@Karrenbelt
Copy link
Author

Karrenbelt commented Nov 17, 2022

current remaining failures:

FAILED tests/test_configurations/test_base.py::test_public_id_comparator_when_author_is_different
FAILED tests/test_configurations/test_base.py::test_public_id_comparator_when_name_is_different
FAILED tests/test_configurations/test_data_types.py::test_self_type_comparison[Dependency]
FAILED tests/test_configurations/test_data_types.py::test_package_version_comparison
FAILED tests/test_configurations/test_data_types.py::test_public_id_comparison

I'm addressing here in this PR:

  • I need to change the error message and test to check for TypeError rather than ValueError.
  • The third is an issue with the test, Dependency does not have a __lt__ method so should not be tested. PackageVersion however is not part of this test and should be. There is another issue with this test in that is generates two examples instead of one, we should only copy otherwise fields like author and name may differ which should raise a TypeError instead of returning a boolean (EDIT: see comment below).

Addressing in a subsequent PR:

  • The last two relate the fact that we compare strings instead of tuples, where
    >>> ("1.2.0" < "1.10.0") == (VersionInfoClass.parse("1.2.0") < VersionInfoClass.parse("1.10.0"))
    False
    

Comment on lines 68 to 72
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)
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)

@Karrenbelt
Copy link
Author

two remaining failures are newly introduced tests in the base PR:

FAILED tests/test_configurations/test_data_types.py::test_package_version_comparison
FAILED tests/test_configurations/test_data_types.py::test_public_id_comparison

Delegating comparison to PackageVersion._version for all classes, however - which would be the last step required to address the issues raised in the base PR #428 - will break e.g. the DependencyTree because packages with different author and name can no longer be sorted. This will be dealt with in subsequent PRs

for tree_level in reversed(flat_tree_dirty[:-1]):
flat_tree.append(
sorted(
{package for package in tree_level if package not in dirty_packages}
)
)
dirty_packages += tree_level
return flat_tree

@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2022

Codecov Report

Base: 97.31% // Head: 97.31% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (f269b13) compared to base (24d3fd1).
Patch coverage: 90.90% of modified lines in pull request are covered.

Additional details and impacted files
@@                   Coverage Diff                    @@
##           fix/data_types__eq__     #447      +/-   ##
========================================================
- Coverage                 97.31%   97.31%   -0.01%     
========================================================
  Files                       221      221              
  Lines                     19540    19543       +3     
========================================================
+ Hits                      19016    19018       +2     
- Misses                      524      525       +1     
Flag Coverage Δ
unittests 97.31% <90.90%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aea/configurations/data_types.py 99.43% <90.90%> (-0.28%) ⬇️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

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

@Karrenbelt Karrenbelt changed the title [1.25.0] Add deflection to custom __lt__ and add functools.total_ordering Add deflection to custom __lt__ and add functools.total_ordering Nov 28, 2022
@angrybayblade angrybayblade marked this pull request as draft December 14, 2022 14:27
@Karrenbelt Karrenbelt marked this pull request as ready for review December 18, 2022 16:38
…nLike

Fix/delegate `__lt__` to `PackageVersionLike`
@DavidMinarsch DavidMinarsch merged commit 7281b27 into fix/data_types__eq__ Dec 19, 2022
@DavidMinarsch DavidMinarsch deleted the fix/data_types__lt__ branch December 22, 2022 22:29
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.

None yet

4 participants