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 pip.req_set.prepare_files #1526

Closed
qwcode opened this issue Feb 2, 2014 · 2 comments
Closed

refactor pip.req_set.prepare_files #1526

qwcode opened this issue Feb 2, 2014 · 2 comments
Labels
auto-locked Outdated issues that have been locked by automation type: refactor Refactoring code

Comments

@qwcode
Copy link
Contributor

qwcode commented Feb 2, 2014

opening an issue to state the obvious, that pip.req_set.prepare_files is too large and complicated. It needs to be redone.

It's full of hairy conditonal logic that's based on:

  1. whether it's sdist vs wheel vs bundle vs editable
  2. whether it's a download vs an install
  3. whether it's an upgrade

for #1, I think the answer is to subclass InstallRequirement (for sdist/wheel/editable), or maybe create our own hierarchy of Distribution classes, and move the logic to those classes. bundle is going away soon, so that will help simplify as well.

for #2, I think clearly separating "unpacking" vs "dependency resolution" vs "installing" vs "downloading" would help a lot. download and install both need unpacking and resolution, as independent steps. Note that in the future, we won't need unpacking when using PyPI (but will still need dependency resolution), due to PyPI likely providing metadata outside of the archive.

@rbtcollins
Copy link

Having been poking around heavily at this, I thought I'd capture a couple of thoughts here.

InstallRequirement is stateful - it builds up state, and we can't tell until we've done a bunch of work with it whether we have an sdist/wheel/editable/$newshiny. That makes using InstallRequirement as a subclasssable thing hard. I think the variation along the sdist/wheel/editable etc dimension should be a separate set of classes.

@dstufft
Copy link
Member

dstufft commented Mar 22, 2017

Closing this, I don't think that we're going to forget that this code is horrible and needs to die.

@dstufft dstufft closed this as completed Mar 22, 2017
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation type: refactor Refactoring code
Projects
None yet
Development

No branches or pull requests

3 participants