Skip to content

Fixed issue#949 #4414

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

Merged
merged 5 commits into from
Apr 6, 2017
Merged

Fixed issue#949 #4414

merged 5 commits into from
Apr 6, 2017

Conversation

luojiebin
Copy link
Contributor

Added a way to distinguish between pip installed packages and those from
the system package manager in 'pip list'. Specifically, 'pip list -vv' also
shows the installer of package.

@xavfernandez xavfernandez added the C: list/show 'pip list' or 'pip show' label Apr 4, 2017
@xavfernandez
Copy link
Member

I'm not sure we wan't to keep adding -v for each new column (should we print the license on -vvv then ? ;) ).
Printing the installer in single -v mode might already make sense, or perhaps allow the user to configure somehow the list of columns to show ? Something like pip list --format="columns:pkg,version,installer" ?

@pfmoore
Copy link
Member

pfmoore commented Apr 4, 2017

Yeah, I'm a bit concerned about this too. I don't want to over-generalise just for the sake of it, but adding extra columns isn't really what I'd expect -v to be for.

@luojiebin
Copy link
Contributor Author

To make it simple and functional. Does it make sense to just show both location and installer of a package when "pip list -v" ?

@luojiebin
Copy link
Contributor Author

luojiebin commented Apr 4, 2017

@xavfernandez

pip list --format="columns:pkg,version,installer"

Is this way a little complicated? I guess it's better to keep it as simple as possiple.

@pfmoore
Copy link
Member

pfmoore commented Apr 4, 2017

OK, I'm confused here. The build has failed because a Distribution object has no "installer" attribute, which is correct. How were you planning on getting the installer metadata? You probably want something like Distribution.get_metadata('installer') but you'll need to tidy it up a bit.

Actually, I'm concerned about the scope creep here. As @xavfernandez pointed out, what's to stop someone else asking for another field to be added?

At the end of the day, pip list is a pretty thin wrapper around pkg_resources. At some point we should consider whether it's right to keep adding complexity, and when users should be encouraged to write their own script. After all, pip list -v is little more than

from pkg_resources import working_set
for dist in working_set:
    print(f"{dist.project_name} {dist.version} {dist.location}")

(with a bit of layout added).

@luojiebin
Copy link
Contributor Author

luojiebin commented Apr 4, 2017

@pfmoore Thanks. I read the source code and find it can get 'installer' attribute in 'show' command and make a mistake to think package has the 'installer' attribute.

Yes, I agree. It should stop someone else asking for another field to be added, because it will make the 'list' command completed.

Then, how should we deal with issue#949 ?Does it make sense to just show both location and installer of a package when "pip list -v", then stop add more columns and encourage users write their own script. Because install location and installer are importance information of a package. Or we should close issue#949?

@pfmoore
Copy link
Member

pfmoore commented Apr 4, 2017

The comments in #949 note that pip show --verbose <pkg> already shows the installer flag. I'm OK with making pip list -v show both the location and the installer flag, as you suggest, but I'm not actually sure it's needed. I've asked on #949 if what's already available is sufficient.

BTW, I just noticed that get_metadata('installer') raises an exception if there's no installer metadata, so you have to be prepared for that. It's not that it's hard to handle, but it does demonstrate that this change isn't quite as trivial as it first seemed... ;-)

@luojiebin
Copy link
Contributor Author

@pfmoore Yes, it is not simple. I will think it again. Thanks for advice patiently.

@@ -237,8 +244,9 @@ def output_package_listing(self, packages, options):
elif options.list_format == 'freeze':
for dist in packages:
if options.verbose >= 1:
logger.info("%s==%s (%s)", dist.project_name,
dist.version, dist.location)
logger.info("%s==%s (%s) (%s)", dist.project_name,
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 not sure it makes sense to include the installer (and the location, fwiw) in the freeze format.

Copy link
Contributor Author

@luojiebin luojiebin Apr 6, 2017

Choose a reason for hiding this comment

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

I think it's reasonable to remove installer from freeze format too.

if options.outdated:
info['latest_version'] = six.text_type(dist.latest_version)
info['latest_filetype'] = dist.latest_filetype
data.append(info)
return json.dumps(data)


def get_installer(dist):
Copy link
Member

Choose a reason for hiding this comment

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

This function should go to pip.utils.packaging

@xavfernandez xavfernandez merged commit d0c0cc9 into pypa:master Apr 6, 2017
@xavfernandez
Copy link
Member

@luojiebin Thanks 👍

@xavfernandez xavfernandez added this to the 10.0 milestone Apr 6, 2017
@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 C: list/show 'pip list' or 'pip show'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants