-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Abstract away AbstractDistribution in higher-level resolver code #8629
Conversation
One potential advantage I can think of to return an |
This is going the wrong direction, IMO. If we're trying to trade out pkg_resources and hide details about how distribution metadata is retrieved, then we need a wrapper around the underlying Distribution. |
@chrahunt #8114 introduces that wrapper. IMO it makes sense for us to migrate away from returning AbstractDistribution out of RequirementPreparer -- we always call |
I'd like to point out #8448 as an example of a clean approach to the kind of issues we're facing in #8638. I think this is a promising direction design-wise, and it depends on us having our own type to contain the "downloaded" state (AbstractDistribution). Any approach that lets us hide some details from the resolver (a plus in my book) will probably take a similar form. I see how this change makes things a little simpler, but I think leaving AbstractDistribution in place for now will help guide us towards approaches that are simpler overall. |
I can get behind this sentiment. FWIW, I'd spotted this simplification a while back when I'd created the |
I missed (at least) one thing here: if we want to persist state that the Candidate and Resolver don't know about, we can always do it on the |
I sense implied approvals for this approach from @pradyunsg and @uranusjr, and likely my concerns holding this back, so I can merge it. Thank you for your patience @McSinyx! |
Thanks for the merge and the explanation from everyone here. |
This (hopefully) simplifies the implementation and makes the logic a bit more obvious.