Add allow_all_prereleases to CandidateEvaluator's constructor

[cjerdonek]

This PR is another follow-up to PR #6424 (the previous follow-up was PR #6511). It adds allow_all_prereleases to the list of arguments accepted by CandidateEvaluator's constructor, as suggested first in this comment to PR #6424: #6424 (comment) Doing this lets us eliminate another argument to PackageFinder's contructor, since it's now taken care of by the candidate_evaluator argument.

This PR also simplifies CandidateEvaluator by moving the creation of InstallationCandidate objects back to PackageFinder, which was made possible by the refactor in PR #6484.

In this way, CandidateEvaluator takes on more of the evaluating / filtering responsibility, while the state stays with PackageFinder.

[uranusjr]

Is there a reason to introduce make_found_candidates()? It’s used only once and has minimal content; I wonder if it is more readable to inline it in PackageFinder.find_candidates().

[cjerdonek]

I don't think "introduced" is the right word because the method already existed before as FoundCandidates.from_specifier(). Rather, the method is being moved / renamed to the CandidateEvaluator class, where the allow_prereleases attribute is located (needed for the method).

But yes, I think there are good reasons to put it there. For one, it hides complexity away from PackageFinder, which we are trying to simplify. For example, with this form, PackageFinder doesn't have to know about allow_prereleases (which is the responsibility of CandidateEvaluator). It also puts the responsibility of filtering with CandidateEvaluator, which is taking on the responsibility of evaluating and sorting candidates. Also, to me I think the lines in the method are actually complicated and hard to understand :) , so it's worth keeping that separated out to help my brain (and maybe others'). (E.g. it contains a nested set comprehension with a 7-line comment in the middle!)

Finally, it makes things nice and clean on the PackageFinder side, which is what we should be optimizing for, IMO:

def find_candidates(self, project_name, specifier=None):
    candidates = self.find_all_candidates(project_name)
    return self.candidate_evaluator.make_found_candidates(
        candidates, specifier=specifier,
    )

Would it help to give it a better, more descriptive name (e.g. filter_found_candidates())? That way it would be clearer why CandidateEvaluator has the responsibility.

[uranusjr]

That makes a lot of sense, thanks for the explanation!

Reading the implementation again, I think what bothers me is not moving constructing function, but that FoundCandidates becomes tightly coupled with CandidateEvaluator. FoundCandidates is only supposed to be created by CandidateEvaluator, but the instance needs the creating evaluator to work. My gut feeling is the border of responsibilities between these two classes might be wrong, but I am not sure what the better approach is here.

Maybe it’d be possible to remove FoundCandidates. get_best(), so that FoundCandidates is only responsible for holding a collection of candidates, and CandidateEvaluator is responsible for determining whether each candidate is suitable for install?

[cjerdonek]

Yes, I agree with you re: the relationship between those two classes. There is some circularity which isn't ideal. At the same time, FoundCandidates is such a tiny class (just a convenience "glue" class, really, encapsulating a result), and easy to change later, that it doesn't bother me too much.

Another possibility to break the circularity would be for CandidateEvaluator to inject not its whole self but just its get_best_candidate method object. Then FoundCandidates just sees an opaque function rather than the whole CandidateEvaluator object. But by doing that it just seems we'd be striving for something for the sake of purity.

Re: removing FoundCandidates.get_best(), one problem there is that it would then become awkward in pip_version_check() (the caller would have to know about the evaluator attribute), and I think we want to measure things by how simple they are for the caller (at the higher levels).

[uranusjr]

I agree that passing an opaque callable is not very worthwhile. I think the interfacing problem of FoundCandidates.get_best() can be solved rather inconsequentially, since pip_version_check() is the only outside user of it right now (besides PackageFinder). But that does not really affect the main purpose of this PR (moving prerelease logic into the evaluator), and can wait after this is merged.

[uranusjr]

I pondered a bit and now feel this is a good design. 💡

[cjerdonek]

Okay, thanks much for taking the time to review and discuss.