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 --path to pip freeze to support --target installations #6450

Merged
merged 3 commits into from
May 28, 2019

Conversation

lkollar
Copy link
Contributor

@lkollar lkollar commented Apr 27, 2019

pip freeze does not support installations in a target directory. This change adds the --path argument to pip freeze to enable this functionality.

Closes: #6404

@lkollar lkollar force-pushed the freeze-target branch 3 times, most recently from cf5475e to b8caf9a Compare April 28, 2019 12:39
@uranusjr
Copy link
Member

I understand the option matches install, but pip freeze --target could be confusing. It reads like “freeze the environment into the target location” to me. The idea of freezing a directory is quite good though!

@lkollar
Copy link
Contributor Author

lkollar commented Apr 29, 2019

How about something like -d/--directory or -p/--path?

Another option would be to take one or more paths as positional arguments. So omitting that (the current behaviour) freeze takes the current environment, otherwise the specified locations. It changes the semantics of pip freeze somewhat though, so it could be confusing as well.

@uranusjr
Copy link
Member

-d would work for me, but this will need to be decided by a pip maintainer 😃 (maybe they will decide --target is good enough!)

@lkollar lkollar changed the title Add support for --target in pip freeze Add support for --path in pip freeze May 6, 2019
@lkollar
Copy link
Contributor Author

lkollar commented May 6, 2019

Changed the argument from --target to --path.

@lkollar lkollar force-pushed the freeze-target branch 3 times, most recently from 380e7ad to c44c9f7 Compare May 6, 2019 15:15
@cjerdonek cjerdonek added C: freeze 'pip freeze' related type: enhancement Improvements to functionality labels May 7, 2019
@lkollar lkollar force-pushed the freeze-target branch 3 times, most recently from 70a7f3b to d9935f3 Compare May 7, 2019 20:11
Copy link
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

Thanks, @lkollar. This PR is well done. I have a few simple suggestions.

Also, @uranusjr, do you have any further comments?

'--path',
dest='path',
action='append',
help='Use the specified installation path for listing packages')
Copy link
Member

Choose a reason for hiding this comment

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

I would end this with "(can be used multiple times)", and then end with a period. I also think it might be clearer to begin the sentence with "Restrict to the ..." or "Use only ..." to make clear that these paths aren't being considered in addition to what is already considered.

tests/functional/test_freeze.py Show resolved Hide resolved
tests/functional/test_freeze.py Show resolved Hide resolved
@@ -0,0 +1,2 @@
Support ``--target`` installations with ``pip freeze`` with the ``--path``
argument.
Copy link
Member

Choose a reason for hiding this comment

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

Here is a slight rewording that I think might be better: "Add a --path argument to pip freeze to support --target installations."

@cjerdonek cjerdonek changed the title Add support for --path in pip freeze Add --path to pip freeze to support --target installations May 17, 2019
Copy link
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

Thanks for the update! A couple clarifications of my previous comments.

tests/functional/test_freeze.py Show resolved Hide resolved
tests/functional/test_freeze.py Show resolved Hide resolved
@lkollar
Copy link
Contributor Author

lkollar commented May 27, 2019

@cjerdonek sorry, I misunderstood your first round of comments on the tests. Submitting another fixup to correct these.

@xavfernandez
Copy link
Member

Since pip freeze and pip list --format=freeze produce almost the same result, maybe pip list should also gain the same argument ?

@cjerdonek
Copy link
Member

@xavfernandez That sounds good. Are you okay with that being handled by a different issue / PR though?

@xavfernandez
Copy link
Member

@xavfernandez That sounds good. Are you okay with that being handled by a different issue / PR though?

Sure

@cjerdonek
Copy link
Member

Thanks for all your good work on this, @lkollar, and for your patience since PyCon. :) Looks great! 👍

@cjerdonek cjerdonek merged commit 929fefd into pypa:master May 28, 2019
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 27, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 27, 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: freeze 'pip freeze' related type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support --target installations with pip freeze
4 participants