Add CandidateEvaluator class

[cjerdonek]

This is to further simplify the PackageFinder class.

[cjerdonek]

@uranusjr This PR is something that occurred to me when reviewing one of your earlier PR's. A subsequent PR could be to change PackageFinder to accept a CandidateEvaluator object instead of the five arguments of versions, platform, abi, implementation, and prefer_binary. Similar to FoundCandidates, there can be a separate "create" function or method, if necessary, accepting these additional arguments.

[cjerdonek]

In addition, if CandidateEvaluator becomes responsible for creating the FoundCandidates object, passing a CandidateEvaluator object can replace passing not just five but six arguments to PackageFinder -- the five I mentioned above as well as allow_all_prereleases.

[uranusjr]

This sounds like a great idea! I’ve been thinking about splitting PackageFiner in three steps: Find all links, filter out applicable links, and choose the best out of them. This is definitely going the same direction, refactoring out the final part out of it.

I might start trying to split index.py into its own package so I can use it in other projects, e.g. Pipenv…

[cjerdonek]

Great, I'm glad you like it! Do you approve of this PR so it can be merged?

Regarding index.py, I would really rather keep refactoring it in place from where it is now IMO as there is a large amount of work that can be done to break it down and decouple it from pip's innards, etc. I think it is a series of easy, incremental steps.

[uranusjr]

Regarding index.py, I would really rather keep refactoring it in place from where it is now IMO as there is a large amount of work that can be done to break it down and decouple it from pip's innards, etc. I think it is a series of easy, incremental steps.

I agree. What I meant is I could try to get a better idea what else needs to be done 😃

[pradyunsg]

I like the direction you're taking this @cjerdonek @uranusjr! :D

I'll bring up the idea that it might also make sense to make index into a sub-package within pip and spread out components within that sub package.

[cjerdonek]

Thanks, @uranusjr and @pradyunsg!

[cjerdonek]

By the way, re: splitting things out further, I noticed that _link_package_versions() and _package_versions() and their supporting functions can be moved to become methods of CandidateEvaluator with very little change.

I believe that would bring the size of the PackageFinder class down to about 450 lines.

[uranusjr]

My gut feeling is they should be in their own class (CandidateFinder or something)? The candidate selection consists of three steps: collecting links, filtering applicable, and select best. FoundCandidates deals the last, CandidateEvaluator the second, so maybe the first should be its own component as well.

[cjerdonek]

I think the methods I mentioned aren’t named well. They’re also responsible for filtering (no “collection” takes place in them). And that filtering also depends exactly on the set of valid tags which CandidateEvaluator already has. It will be nice because it will collect together all the methods that filter and sort by the valid tags. It will be a very functional (in the sense of pure function), testable class.

[cjerdonek]

The candidate selection consists of three steps: collecting links, filtering applicable, and select best. FoundCandidates deals the last, CandidateEvaluator the second,

Also, I think it might be worth broadening how we think about the process beyond how it is currently structured because I feel like a number of simplifications might be possible. For example, "filtering" is currently also done inside "select best," as well as when collecting links, so the responsibilities are spread across which isn't so good. As another example, I think there is a chance that the filtering done inside "select best" could be happening a lot earlier in the process (even before the InstallationCandidate objects are created).

[cjerdonek]

I just posted PR #6425 to do part of what I suggested in my previous comment. It moves _link_package_versions() to CandidateEvaluator (but not the other methods). It also renames _link_package_versions() to evaluate_link() to make clearer what the method actually does (evaluate whether a link is a candidate for install).

[uranusjr]

Reading the source, I think the functions are probably both inadaquently named and implemented (in the three-step sense). _link_package_versions() does filtering (based on search.formats etc.), but is also responsible for excluding href values that don’t point to a package. I would hope the latter happens during the finding step instead. Maybe some further refactoring can happen before (or after) the PR?

[cjerdonek]

Currently, the _link_package_versions() method's only dependency is on the list of valid tags, so I think it's an improvement to move that method to the class that encapsulates the valid tags. (Indeed, _link_package_versions()'s only method call is to call a method on the CandidateEvaluator class.) It also cuts down the size of the PackageFinder class and reduces what it has to "know about," which makes it easier to reason about. We should definitely keep refactoring after any PR (that's what I was advocating above). I wasn't suggesting this would be the final place for anything to live. :) By refactoring things in easy, incremental steps, each step will be an improvement but not necessarily the way we want it in the end.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.