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

Add pip download command and deprecate pip install --download. #3085

Merged
merged 1 commit into from
Sep 15, 2015

Conversation

patricklaw
Copy link

This is a followup to #2995

action='store_true',
default=False,
help="Include pre-release and development versions. By default, "
"pip only finds stable versions.")
Copy link
Member

Choose a reason for hiding this comment

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

@patricklaw
Copy link
Author

Thanks for the quick review, @xavfernandez . First three issues were corrected.

@@ -139,7 +139,7 @@ def test_download_vcs_link(script):
It should allow -d flag for vcs links, regression test for issue #798.
"""
result = script.pip(
'install', '-d', '.', 'git+git://github.com/pypa/pip-test-package.git'
'download', '-d', '.', 'git+git://github.com/pypa/pip-test-package.git'
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering whether we should duplicate all those tests to keep checking for the 2 next versions that the existingpip install --download ... commands keep working along with the new pip download command...

Copy link
Member

Choose a reason for hiding this comment

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

And the more I think about it, the more I think we should.
New features might be added to pip download without affecting the current pip install --download behavior and having tests will help ensure we do not break anything :)

Copy link
Author

Choose a reason for hiding this comment

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

Done!

@patricklaw patricklaw force-pushed the add-pip-download-command branch from ed810ea to 3d2f45c Compare September 9, 2015 03:13
@patricklaw patricklaw changed the title Add pip download command. This has the same functionality that pip in… Add pip download command and deprecate pip install --download. Sep 9, 2015
'-d', '--dest', '--destination-dir', '--destination-directory',
dest='download_dir',
metavar='dir',
default=None,
Copy link
Member

Choose a reason for hiding this comment

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

I think this one should default to something like the current directory or maybe ./pip_downloads (to mimic the way pip wheel works, cf https://github.com/pypa/pip/blob/develop/pip/commands/wheel.py#L18).

With the default being None, pip download something currently deletes everything it has downloaded.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

So I don't have a strong opinion on this, but here's a question... Does it make sense to have a default directory? What if we made the CLI pip download <directory> <pkgs>? Is that better or worse?

I don't know! I certainly don't think having a default is wrong, but I'm curious if we're having a default just to have one when we really expect it to always be specified?

Copy link
Member

Choose a reason for hiding this comment

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

From my point of view, pip download is very similar to pip wheel so I expect it to behave the same.
I'm not a fan of pip download <directory> <pkgs>, I'd rather use pip download -d <directory> <pkgs>, not much longer but way clearer.
An other (bad) reason we need a default: currently if download_dir is None, the RequirementSet won't download/keep anything. ^^

'-d', '--dest', '--destination-dir', '--destination-directory',
dest='download_dir',
metavar='dir',
default=DEFAULT_DOWNLOAD_DIR,
Copy link
Member

Choose a reason for hiding this comment

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

Better but the directory is not automatically created and a simple pip download setuptools crashes.
We need to create the dir if it doesn't exist, I'd again copy the pip wheel command behavior, cf https://github.com/pypa/pip/blob/develop/pip/req/req_set.py#L330-L331

Copy link
Member

Choose a reason for hiding this comment

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

Ideally with a new test case for the download command without --download-dir option

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, and test added.

@patricklaw patricklaw force-pushed the add-pip-download-command branch from e8cfbe8 to b04ba0d Compare September 10, 2015 20:21

.. pip-command-usage:: download

``pip download`` is used just like ``pip install --download`` was used in the past. The same rules apply to this command: it should be very comfortable to use.
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be documented on it's own and not in relation to pip install --download which will be deprecated and removed.

Copy link
Author

Choose a reason for hiding this comment

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

Clarified the documentation. I noted explicitly that this replaces pip install --download, but that the latter is deprecated and will be removed. I'm not very familiar with this doc formatting, so if there's any extra cross-linking I should be doing to pip install, please point me at an example of how to do so.

`pip download` has the same functionality as `pip install --download`,
and the behavior of `pip install --download` is preserved with a deprecation
warning.  `pip install --download` will be removed in pip version 10.
@patricklaw patricklaw force-pushed the add-pip-download-command branch from b04ba0d to 417f79d Compare September 13, 2015 21:41
xavfernandez added a commit that referenced this pull request Sep 15, 2015
Add `pip download` command and deprecate `pip install --download`.
@xavfernandez xavfernandez merged commit 0b1acf9 into pypa:develop Sep 15, 2015
@xavfernandez
Copy link
Member

Thanks for your work ! 👍

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants