Add support for "yanked" files (PEP 592)

[cjerdonek]

Fixes #6633.

[pradyunsg]

Wohoo! @cjerdonek Thanks for picking this up! :D

I'd prefer if we model this a little differently -- part of this is from the PoV that when we do have backtracking resolution, we'd want an order of preference, rather than just a single choice as returned currently. CandidateEvaluator / FoundCandidates are a perfect model for that as it stands IMO, so I'd like to keep it that way. :)

We should filter the items in case of allow_yanked=False, in iter_applicable(). In _sort_key, we should move yanked releases to the end of the sorted list. That way, get_best_candidate can change to return sorted_candidates[0] / None if no candidates match -- or go back to max.

Yes, this splits the responsibility of handling yanked releases, across two classes but I feel that is justified since CandidateEvaluator should only hold the logic for "which is best" and FoundCandidates should hold the logic for "which are allowed/OK".

Thoughts welcome as always. :)

---

aside: About the sorted call in get_best_candidate -- As it stands, I'd figured it's easy to modify it to simply expose the sorted list of candidates from CandidateEvaluator, instead of the best candidate. The backtracking resolver will want a sequence of candidates by order of preference, so if we're making the change of storing the sorted list and then "choosing" the best, it'd probably be worthwhile to put the "sort this candidate list" in a method that we can start adding tests for it eagerly. I'm okay to defer that to later and go back to the max call for now (there's no hurry and it'd be a good task for new contributors too FWIW).

[xavfernandez]

Nice and clear as usual 👍
Maybe a more functional test could be added with a fake index (something like https://github.com/pypa/pip/tree/master/tests/data/indexes)

[pradyunsg]

Nice and clear as usual 👍

+1

I don't say this often enough -- your PRs are super smooth to review @cjerdonek! :D

[xavfernandez]

In _sort_key, we should move yanked releases to the end of the sorted list. That way, get_best_candidate can change to return sorted_candidates[0] / None if no candidates match -- or go back to max.

👍 I did not like the need to sort the whole list but could not figure a clean way to do that 🤦‍♂️ .

We should filter the items in case of allow_yanked=False, in iter_applicable()

It crossed my mind, but was uncomfortable with the fact that allow_yanked would be used in two places.
But your arguments are sound and I've no simple alternative that crosses my mind 🤷‍♂️

[cjerdonek]

In _sort_key, we should move yanked releases to the end of the sorted list.

Awesome suggestion! Thanks for pointing that out. My mind was thinking of a related PR, so I missed that way of thinking about it.

[cjerdonek]

Thanks, @pradyunsg and @xavfernandez for the great suggestions and quick reviews. I just pushed an updated PR that I think incorporates all of your suggestions. I also think the PR and tests have become simpler due to @pradyunsg's suggestion on sorting vs. filtering.

The only thing missing is a more functional / end-to-end test as @xavfernandez suggested, but I can start thinking about how to do that.

[cjerdonek]

Since @xavfernandez and @pradyunsg both approved this, and because the other approach / refactor we started discussing (re: filtering out yanked files earlier in the allow_yanked=False case) isn't trivial, it's probably best to address that in a follow-on PR or PR's. That would also let us discuss and experiment a bit more, without holding up the bulk of things. The more functional, end-to-end test that @xavfernandez mentioned is something else that could go in either as part of that or in another follow-on PR, especially since a higher-level test would be more useful / needed with the other approach. (And there is already quite a number of tests added in this PR.)

[pradyunsg]

Sounds good to me! :)

[cjerdonek]

Thanks again, @pradyunsg and @xavfernandez for your good suggestions and reviews!

[pradyunsg]

Just a minor process thing -- it'd be great if you could file issues for the follow ups to be made here, the end-to-end test as well as the filtering discussion! :)

Thanks for doing this @cjerdonek!

[cjerdonek]

Done here (#6653) and here (#6654).

[pradyunsg]

Yay! :)

Thanks for doing this @cjerdonek!

With the power of re-reading, a minor clarification: this = this PR; not the issues. But, but, but thanks for making them @cjerdonek! Much appreciated! :)