Skip to content
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

refactor requirement preparer to remove duplicated code paths for metadata-only dists #12871

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cosmicexplorer
Copy link
Contributor

@cosmicexplorer cosmicexplorer commented Jul 22, 2024

This PR is on top of #12863; see the +181/-153 diff against it at https://github.com/cosmicexplorer/pip/compare/dist-caching-changes...cosmicexplorer:pip:refactor-requirement-preparer?expand=1.

Problem

There is a lot of duplicated code in RequirementPreparer for the metadata-only and "normal" code paths. Regarding @pfmoore's analysis from #12186 (comment):

I'm concerned that the preparer is one of the least "obvious" parts of the resolution process - it's full of generic terms (not least of which is "prepare" itself!) which don't really give much clue as to what precisely is going on. Every time I need to deal with the preparer, I have to do a bunch of research to remind myself what's happening before I can proceed. I'm worried that if we delegate chunks of the "prepare" task to individual commands, we will end up making that problem worse, with everyone needing to keep track of what's happening.

Solution

  • Split out a lot of logic from _prepare_linked_requirement() into private helper methods.

Result

Less duplicated code, and hopefully RequirementPreparer will be easier to maintain.

@cosmicexplorer cosmicexplorer force-pushed the refactor-requirement-preparer branch 2 times, most recently from f4c8812 to 86b8668 Compare July 22, 2024 23:19
@cosmicexplorer cosmicexplorer changed the title Refactor requirement preparer refactor requirement preparer to remove duplicated code paths for metadata-only dists Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant