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.

Sign up for GitHub

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

Jump to bottom

Refactor pip.commands.install #2592

Closed
wants to merge 2 commits into from
Closed

Refactor pip.commands.install #2592

wants to merge 2 commits into from

Conversation

msabramo
Copy link
Contributor

@msabramo msabramo commented Mar 20, 2015 •
edited by BrownTruck
Loading

I find them a little less distracting when collected in one place, rather than sprinkled around.

qwcode
qwcode reviewed Mar 20, 2015
View reviewed changes
pip/commands/install.py
" is suggested.",
RemovedInPip7Warning,
)
index_urls += options.mirrors
Copy link
Contributor

Choose a reason for hiding this comment

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

this local var is disconnected now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scary part is the tests didn't fail...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the issue and it inspired a few more refactorings

@msabramo msabramo force-pushed the install_centralize_deprecations branch from 53b1d24 to de49649 Compare March 20, 2015 18:24
@msabramo msabramo changed the title Centralize install deprecations in one place Refactor pip.commands.install Mar 20, 2015
@msabramo
Copy link
Contributor Author

Updated. Tests passing.

@xavfernandez
Copy link
Member

Code does seem clearer. I like the get_index_urls, get_install_options and get_temp_target_dir methods but prefer to see the warning next to the code that will be removed.
With maybe a check_for_deprecated_options for no-op options like use_mirrors or download_cache.

@msabramo msabramo force-pushed the install_centralize_deprecations branch from 2f70d7a to a647efb Compare March 23, 2015 15:33
@msabramo
Copy link
Contributor Author

@xavfernandez: I agree. Updated.

@msabramo msabramo force-pushed the install_centralize_deprecations branch from a647efb to e025a79 Compare March 23, 2015 16:10
@msabramo
Copy link
Contributor Author

tests passing

@dstufft
Copy link
Member

dstufft commented Apr 7, 2015

This will need to be rebased (sorry!), a lot of the deprecated stuff is gone now.

@msabramo
Centralize install deprecations in one place
e3a9383 
I find them a little less distracting when collected in one place,
rather than sprinkled around.
@msabramo
Extract more methods from InstallationCommand.run
2bb8657 
More modular, easier to understand, easier to test, etc.
@msabramo msabramo force-pushed the install_centralize_deprecations branch from e025a79 to 2bb8657 Compare April 10, 2015 16:53
@msabramo
Copy link
Contributor Author

Just rebased this and the Travis CI tests are passing, so ready for another round of review.

@dstufft dstufft closed this May 18, 2016
@dstufft
Copy link
Member

dstufft commented May 18, 2016

Accidentally closed this, reopening. Sorry!

@dstufft dstufft reopened this May 18, 2016
@BrownTruck
Copy link
Contributor

Hello!

As part of an effort to ease the contribution process and adopt a more standard workflow pip has switched to doing development on the master branch. However, this Pull Request was made against the develop branch so it will need to be resubmitted against master. Unfortunately, this pull request does not cleanly merge against the current master branch.

If you do nothing, this Pull Request will be automatically closed by @BrownTruck since it cannot be merged.

If this pull request is still valid, please rebase it against master (or merge master into it) and resubmit it against the master branch, closing and referencing the original Pull Request.

If you choose to rebase/merge and resubmit this Pull Request, here is an example message that you can copy and paste:

I find them a little less distracting when collected in one place, rather than sprinkled around.

---

*This was migrated from pypa/pip#2592 to reparent it to the ``master`` branch. Please see original pull request for any previous discussion.*

@BrownTruck
Copy link
Contributor

This Pull Request was closed because it cannot be automatically reparented to the master branch and it appears to have bit rotted.

Please feel free to re-open it or re-submit it if it is still valid and you have rebased it onto master or merged master into it.

@BrownTruck BrownTruck added auto-bitrotten PRs that died because they weren't updated and removed asked to reparent labels May 26, 2016
@BrownTruck BrownTruck closed this May 26, 2016
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
auto-bitrotten PRs that died because they weren't updated auto-locked Outdated issues that have been locked by automation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@msabramo @xavfernandez @dstufft @BrownTruck @qwcode