sarugaku / resolvelib

Resolve abstract dependencies into concrete ones
ISC License
142 stars 31 forks source link

Candidate filtering depends on candidate equality logic implicitly #68

Closed uranusjr closed 3 years ago

uranusjr commented 3 years ago

It should not. This currently works in pip because pip’s implementation caches candidates so equality works via object identity, and in tests because all tests currently use tuples (or namedtuples) to represent candidates.

Some potential solutions:

webknjaz commented 3 years ago
  • Explicitly require the implementer to supply Candidate.__eq__ (and __ne__ on Python 2). I don’t like this.

FWIW this sounds the most appealing to me.

pfmoore commented 3 years ago

Note that a candidate need not actually be a class at all - the resolvelib API is currently very careful to not impose any constraints on what a "candidate" or a "requirement" actually is. A case-insensitive string is a perfectly valid option, in theory.

It does feel to me as though that design is becoming problematic, the fact that we need to even consider Provider.is_equal_to suggests that having started with a more explicitly object-oriented design might have been easier. But at this point, reworking the design to that extent would be non-trivial.

IMO, Provider.excluding() is the interface that best fits with the existing design

uranusjr commented 3 years ago

It just came to me that maybe candidate equality can be avoided altogether instead. Candidates are iterated through in a fixed order, so incompatibilities should be recorded by their indexes in the iterator instead of the candidate object themselves. That would be best if doable.

uranusjr commented 3 years ago

It didn’t work out. Provider.find_matches() at different level may return candidates with different indexes (because of direct URLs discovered deeper in the stack), so the indexes do not stay consistent to be used for lookup. Back to the top.

pradyunsg commented 3 years ago

IMO, Provider.excluding() is the interface that best fits with the existing design

+1 to this, from me as well.

uranusjr commented 3 years ago

I came up with a fourth approach in #70, adding an argument to find_matches() for candidate exclusion.