Add `hash`/`eq` to requirements #499

abravalheri · 2022-01-18T21:33:16Z

Hello, this PR is a feature request that started as a conversation in #498 (comment). I also believe it might close #453.

The idea here is to be able to compare requirement objects as well as be able to compare sets containing those objects, e.g.:

Requirement("packaging") == Requirement("packaging")
{Requirement("packaging"), Requirement("appdirs")} == {Requirement("packaging"), Requirement("appdirs")}

The approach I used was to rely on the normalization that happens when the requirement object is converted to string to implement both __eq__ and __hash__.

docs/requirements.rst

packaging/requirements.py

abravalheri · 2022-01-21T13:07:27Z

Thank you very much @brettcannon for the review, I have submitted some commits addressing the proposed changes.

Please note that in order to circumvent the str conversion during the comparison of Requirement, I had to make Marker also comparable and this comparison does impose some processing cost (please let me know if you have any suggestion to prevent that cost).

Alternatively we could still make use of str in Marker.__eq__.

docs/requirements.rst

abravalheri · 2022-01-21T17:07:03Z

packaging/markers.py

+        if not isinstance(other, Marker):
+            return NotImplemented
+
+        return _flatten_marker(self._markers) == _flatten_marker(other._markers)


An alternative to flattening _markers here would be doing it directly on the __init__ method (that could potentially also simplify the _format_marker function)

As in pre-compute the string? Is the string used enough to warrant the forced cost of doing that?

Not necessarily the string. The key point here seems to be flattening parenthesized groups with a single element (or the data structure equivalent to the parsing of those).

The "flattening" seems to be one of the central keys of the string conversion too.

We could do this flattening in the __init__ function without pre-computing the string, and that would facilitate both __eq__ and __str__.

My vote is to rely on the string representation for simplicity, else we are duplicating algorithms for walking the markers. If it turns out performance is a problem we can cache it, but we can wait on that until that actually becomes a problem.

Yeah, I think that definitely is the simplest approach. I have updated the implementation accordingly.

(To be honest my first impulse was to do a string comparison, but I refrained from adopting that by overthinking about a previous comment, in a different context, about the cost of the string conversion).

brettcannon

I'm not sure what anyone thinks about accepting this overall, but if this were to get accepted I think the hash tests should be a bit more clear as to what they are testing for.

packaging/markers.py

brettcannon · 2022-01-27T20:20:37Z

packaging/markers.py

+        if not isinstance(other, Marker):
+            return NotImplemented
+
+        return _flatten_marker(self._markers) == _flatten_marker(other._markers)


As in pre-compute the string? Is the string used enough to warrant the forced cost of doing that?

tests/test_markers.py

tests/test_requirements.py

abravalheri · 2022-01-28T01:56:21Z

Hi @brettcannon, thank you very much for the review. Regarding the hashing test, I was really unfortunate with the naming since it does not reflect what I had in mind.

Something important to say is that, the idea of the test was not to check if the objects are correctly being hashed... To be sincere I think all of that is just an implementation detail. The real objective of the tests were to check if the objects can be used as elements of sets and if comparisons would work fine in that context. Knowing that the output of the hash() function matches does not seem to bring a lot of value to me... (let's hypothesise that Python 16 decides to change how set comparisons work, testing the output of hash() could not necessarily imply that the functionality I am after would keep working).

Please let me know if clarifying the names of the tests and splitting them up acordingly would work for you and I will do my best to come up with better names. Otherwise I also don't have a problem and just simplifying the tests to use hash().

brettcannon · 2022-01-28T21:17:31Z

I actually think the opposite of you. 😄 I view the ability to put an object in a container a side-effect of implementing __hash__ and __eq__.

Now if you want to consolidate testing both methods into testing if the objects get put into a container as appropriate, then that's fine, just make sure to name and document the tests appropriately. I don't have strong opinions either way, but I think it should be one or the other approaches.

abravalheri · 2022-01-31T10:39:30Z

Thank you very much for the comment @brettcannon.

I think it is important to test directly __eq__ (that should be by far the most common use case motivating this change), so I went with your suggestion of testing with the hash() function in my last commit.

brettcannon

I think we should keep this simple and treat the string representation as the simple encoding for comparison.

Also some test simplification to save on some time and electricity.

brettcannon · 2022-03-08T20:30:45Z

packaging/markers.py

+    assert isinstance(marker, (list, tuple))
+
+    if isinstance(marker, tuple):
+        return marker
+
+    if len(marker) == 1:
+        return _flatten_marker(marker[0])
+
+    return [_flatten_marker(e) if isinstance(e, list) else e for e in marker]


Suggested change

assert isinstance(marker, (list, tuple))

if isinstance(marker, tuple):

return marker

if len(marker) == 1:

return _flatten_marker(marker[0])

return [_flatten_marker(e) if isinstance(e, list) else e for e in marker]

assert isinstance(marker, (list, tuple))

if isinstance(marker, tuple):

return marker

elif len(marker) == 1:

return _flatten_marker(marker[0])

else:

return [_flatten_marker(e) if isinstance(e, list) else e for e in marker]

brettcannon · 2022-03-08T20:37:30Z

packaging/markers.py

+        if not isinstance(other, Marker):
+            return NotImplemented
+
+        return _flatten_marker(self._markers) == _flatten_marker(other._markers)


My vote is to rely on the string representation for simplicity, else we are duplicating algorithms for walking the markers. If it turns out performance is a problem we can cache it, but we can wait on that until that actually becomes a problem.

tests/test_markers.py

brettcannon · 2022-03-08T20:39:02Z

tests/test_markers.py

+        # Markers should not be comparable with other kinds of objects.
+        assert marker1 != example1


This can be its own test instead of having to do it repeatedly as part of a parameterized test.

brettcannon · 2022-03-08T20:39:54Z

tests/test_requirements.py

+        # Requirement objects should not be comparable with other kinds of objects.
+        assert req1 != dep1
+        assert req2 != dep2


Can be a separate test that isn't repeated multiple time needlessly.

Co-authored-by: Brett Cannon <brett@python.org>

Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com>

Co-authored-by: Brett Cannon <brett@python.org>

abravalheri · 2022-03-15T11:11:25Z

Thank you very much for the updated reviews.

I have adopted the suggested changes and also rebased the PR.

packaging/markers.py

brettcannon · 2022-03-15T23:44:22Z

Thanks for much for sticking with this, @abravalheri !

abravalheri marked this pull request as ready for review January 18, 2022 21:46

brettcannon added packaging.requirements enhancement labels Jan 19, 2022

brettcannon requested changes Jan 19, 2022

View reviewed changes

docs/requirements.rst Outdated Show resolved Hide resolved

packaging/requirements.py Outdated Show resolved Hide resolved

abravalheri force-pushed the hashable-requirements branch from 01038fa to 3005d18 Compare January 21, 2022 13:03

pradyunsg reviewed Jan 21, 2022

View reviewed changes

docs/requirements.rst Outdated Show resolved Hide resolved

abravalheri commented Jan 21, 2022

View reviewed changes

brettcannon self-requested a review January 22, 2022 00:03

pradyunsg self-requested a review January 25, 2022 00:31

brettcannon requested changes Jan 27, 2022

View reviewed changes

brettcannon self-requested a review March 8, 2022 03:06

brettcannon requested changes Mar 8, 2022

View reviewed changes

abravalheri and others added 14 commits March 15, 2022 11:05

Make Requirement hashable

b8b127b

Add tests for hashable/comparable requirements

17c7d93

Document comparisons between requirement objects

d4976f3

Fix misspelling in docs/requirements.rst

8bcac29

Co-authored-by: Brett Cannon <brett@python.org>

Make markers hashable/comparable

61929e6

Replace string comparison with attributes for requirements

cdd6569

Document comparisons between marker objects

1473075

Allow comparison between subclasses of Requirement and Marker

85f8403

Update docs/requirements.rst

dad5903

Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com>

Update docs/markers.rst

c32d640

Remove unecessary type cast

0d97221

Test hashes directly

295f2c3

Use string comparisson for markers instead of flattening

e94fce0

Separate tests comparing objects to markers and requirements

0a6559f

Apply suggestions from code review

ed0c623

Co-authored-by: Brett Cannon <brett@python.org>

abravalheri force-pushed the hashable-requirements branch from a942422 to ed0c623 Compare March 15, 2022 11:06

brettcannon self-requested a review March 15, 2022 17:47

brettcannon approved these changes Mar 15, 2022

View reviewed changes

packaging/markers.py Outdated Show resolved Hide resolved

Tweak grammar in a comment

5feae24

brettcannon merged commit aebc072 into pypa:main Mar 15, 2022

pelson mentioned this pull request May 30, 2022

Equality operator not implemented on Requirement #325

Closed

abravalheri deleted the hashable-requirements branch June 7, 2022 15:24

uranusjr mentioned this pull request Aug 31, 2022

Equality is missing in Requirement contrast to what is mentioned in the documentation #588

Closed

j01101111sh mentioned this pull request Dec 8, 2022

Bump packaging from 21.3 to 22.0 j01101111sh/metabase-tools#178

Merged

stanislavlevin mentioned this pull request Mar 13, 2024

Unsupported packaging <22.0 stanislavlevin/pyproject_installer#62

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `hash`/`eq` to requirements #499

Add `hash`/`eq` to requirements #499

abravalheri commented Jan 18, 2022 •

edited

Loading

abravalheri commented Jan 21, 2022 •

edited

Loading

abravalheri Jan 21, 2022 •

edited

Loading

brettcannon Jan 27, 2022

abravalheri Jan 28, 2022

brettcannon Mar 8, 2022

abravalheri Mar 15, 2022

brettcannon left a comment

brettcannon Jan 27, 2022

abravalheri commented Jan 28, 2022 •

edited

Loading

brettcannon commented Jan 28, 2022

abravalheri commented Jan 31, 2022 •

edited

Loading

brettcannon left a comment

brettcannon Mar 8, 2022

brettcannon Mar 8, 2022

brettcannon Mar 8, 2022

brettcannon Mar 8, 2022

abravalheri commented Mar 15, 2022

brettcannon commented Mar 15, 2022

		# Markers should not be comparable with other kinds of objects.
		assert marker1 != example1

Add __hash__/__eq__ to requirements #499

Add __hash__/__eq__ to requirements #499

Conversation

abravalheri commented Jan 18, 2022 • edited Loading

abravalheri commented Jan 21, 2022 • edited Loading

abravalheri Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettcannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abravalheri commented Jan 28, 2022 • edited Loading

brettcannon commented Jan 28, 2022

abravalheri commented Jan 31, 2022 • edited Loading

brettcannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abravalheri commented Mar 15, 2022

brettcannon commented Mar 15, 2022

Add `hash`/`eq` to requirements #499

Add `hash`/`eq` to requirements #499

abravalheri commented Jan 18, 2022 •

edited

Loading

abravalheri commented Jan 21, 2022 •

edited

Loading

abravalheri Jan 21, 2022 •

edited

Loading

abravalheri commented Jan 28, 2022 •

edited

Loading

abravalheri commented Jan 31, 2022 •

edited

Loading