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

Breakout pip._internal.download to a pip._internal.network sub-package #6813

Closed
pradyunsg opened this issue Jul 30, 2019 · 10 comments · Fixed by #7187
Closed

Breakout pip._internal.download to a pip._internal.network sub-package #6813

pradyunsg opened this issue Jul 30, 2019 · 10 comments · Fixed by #7187
Labels
auto-locked Outdated issues that have been locked by automation C: download About fetching data from PyPI and other sources type: refactor Refactoring code

Comments

@pradyunsg
Copy link
Member

pip._internal.download is almost 1200 LoC.

It'd probably make sense to break it up into smaller modules in a pip._internal.network package.

@pradyunsg pradyunsg added the type: refactor Refactoring code label Jul 30, 2019
@cjerdonek
Copy link
Member

I think some of the helper functions (like url_to_path), would be better somewhere inside utils, especially ones that don’t have extra dependencies and are needed for non-download tasks.

@pradyunsg
Copy link
Member Author

Yep yep.

I think it may also make sense to move the utilities that deal with the urls, credentials, and related things, to pip/_internal/network/utils.py or a pip/_internal/utils/network.py (I have a slight preference for the former but don't really care enough either way to want to discuss differences between them).

@cjerdonek
Copy link
Member

My preference has always been the latter (something in utils) because that way it becomes easier to understand and manage our import dependencies. For example, we can have a rule that modules inside utils should have few dependencies, and we can observe and maintain this easily by making sure that modules in utils don't depend on other sub-packages (so it's an inner-most package). With the former, though, the dependencies between sub-packages can look more like spaghetti (modules in network depending on utils and modules in utils depending on modules in network, etc). Things are easier if the dependencies between sub-packages are "one-way."

@cjerdonek
Copy link
Member

cjerdonek commented Aug 11, 2019

FYI, I posted issue #6861 ("Move unpack_file_url() out of download.py"), which is related to this (or perhaps part of it). It's a bit different though because unpack_file_url() is a non-network portion of the functionality.

@cjerdonek cjerdonek added the C: download About fetching data from PyPI and other sources label Aug 11, 2019
@pradyunsg
Copy link
Member Author

@chrahunt Could you link your PRs to this issue?

@chrahunt
Copy link
Member

chrahunt commented Oct 2, 2019

#7045 #7062 #7089

@chrahunt
Copy link
Member

chrahunt commented Oct 2, 2019

download.py is now 578 lines and most of its contents have overlapping concerns (checking hashes, resolving between http/file/vcs urls) that can't be as easily broken out. OK to close this?

@pradyunsg
Copy link
Member Author

I think we should move that functionality into the network package as well.

@chrahunt
Copy link
Member

chrahunt commented Oct 2, 2019

What specifically?

@pradyunsg
Copy link
Member Author

Just, like, I think we should not have a download.py module on the top level when we also have a network sub-package.

It's more of preferences than something technical.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Nov 17, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 17, 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 C: download About fetching data from PyPI and other sources type: refactor Refactoring code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants