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

Remove from pip/_internal/__init__.py the vcs module imports #6545

Merged
merged 2 commits into from Jun 3, 2019

Conversation

cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented May 27, 2019

This is related to #4768 ("Speedup startup time"). This PR removes from pip's main __init__.py the imports from all of the vcs submodules.

Currently, pip's main __init__.py imports all of the vcs submodules. My guess is that it was done this way as the way to make sure that all of the vcs's are registered in the VcsSupport class when any vcs code is first used. (Registering a particular VCS happens when the corresponding vcs submodule is imported.) This PR removes those imports by--

  1. Renaming the current _internal/vcs/__init__.py to _internal/vcs/versioncontrol.py, and
  2. Moving the VcsSupport registrations from _internal/__init__.py to _internal/vcs/__init__.py.
  3. (Also, the definition of path_to_url() had to be moved from the top-level download.py to utils/misc.py to avoid a circular dependency, since at least one of the vcs submodules was importing path_to_url(). This is something that should be done anyways because path_to_url() is used widely throughout the code base.)

@cjerdonek cjerdonek added C: vcs pip's interaction with version control systems like git, svn and bzr skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code labels May 27, 2019
@cjerdonek
Copy link
Member Author

cjerdonek commented May 29, 2019

I pushed a new version of this PR with the file rename moved to its own commit. Now the diff is a lot more readable (when viewing the individual commits).

Here is the main commit (the third and last one shows how the imports changed): 66f55e4

@cjerdonek cjerdonek force-pushed the vcs-imports branch 3 times, most recently from 1426a91 to 66f55e4 Compare May 29, 2019 14:00
@boxed
Copy link

boxed commented May 29, 2019

Nice! Does this do anything significant to the import time? I didn't get any solidly measurable impact until I had done this and lots of other changes. That's why my PR is so big.

@cjerdonek
Copy link
Member Author

cjerdonek commented May 29, 2019

Does this do anything significant to the import time?

No, this PR doesn't change the set of modules imported in the "common case." It's just a preparatory refactor that would make it possible / much easier to remove the vcs imports in a subsequent commit. That can be done in a subsequent PR if we want (e.g. by moving the imports into function calls like you've done).

The current PR is a refactoring change that should be done for independent reasons, IMO.

@cjerdonek
Copy link
Member Author

Hi @xavfernandez, would you be able to take a look at this? It's mostly just renaming a file and updating the affected imports (easiest to see from looking at the three commits individually).

Copy link
Member

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last 2 commits might be squashed together since 331ac9523e on its own surely breaks a lot of things :)

@cjerdonek
Copy link
Member Author

Thanks, @xavfernandez!

Re: squashing those two, I thought git-bisect only looked at merge commits by default, but I guess not when I researched this. Of course, the downside is that once they're squashed, there won't be an easy way for people to see what changed in the file (I don't think). But oh well..

This also renames the current vcs/__init__.py to vcs/versioncontrol.py.
@cjerdonek cjerdonek merged commit ce46f85 into pypa:master Jun 3, 2019
@cjerdonek cjerdonek deleted the vcs-imports branch June 3, 2019 09:10
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jul 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jul 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 C: vcs pip's interaction with version control systems like git, svn and bzr skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants