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

Make pip show less verbose #3858

Merged
merged 5 commits into from Jul 27, 2016

Conversation

Projects
None yet
3 participants
@cdosborn
Contributor

cdosborn commented Jul 20, 2016

Here's an accompanying PR for #3855, which motivates the feature and demonstrates output.

@@ -37,7 +37,8 @@ def run(self, options, args):
query = args
results = search_packages_info(query)
if not print_results(results, options.files):
if not print_results(
results, list_files=options.files, verbose=options.verbose):

This comment has been minimized.

@cdosborn

cdosborn Jul 20, 2016

Contributor

Prefer keyword arguments that are optional.

@cdosborn

cdosborn Jul 20, 2016

Contributor

Prefer keyword arguments that are optional.

Show outdated Hide outdated pip/commands/show.py
Show outdated Hide outdated pip/commands/show.py
@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Jul 20, 2016

Member

You'll need to update the tests (and docs). But in principle, I like this change.

Could you include an example of the new output here? I can see what the code does, but it's easier to visualise the impact of the change with an actual example.

Member

pfmoore commented Jul 20, 2016

You'll need to update the tests (and docs). But in principle, I like this change.

Could you include an example of the new output here? I can see what the code does, but it's easier to visualise the impact of the change with an actual example.

@cdosborn

This comment has been minimized.

Show comment
Hide comment
@cdosborn

cdosborn Jul 20, 2016

Contributor

Here is some sample output: (the issue linked at top contains more info).

pip show pip setuptools
Name: pip
Version: 8.2.0.dev0
Summary: The PyPA recommended tool for installing Python packages.
Home-page: https://pip.pypa.io/
Author: The pip developers
License: MIT
Location: /Users/cdosborn/proj/py/pip_resolver/pip
Requires: 
---
Name: setuptools
Version: 2.2
Summary: Easily download, build, install, upgrade, and uninstall Python packages
Home-page: https://pypi.python.org/pypi/setuptools
Author: Python Packaging Authority
License: PSF or ZPL
Location: /Users/cdosborn/.ve/a/lib/python2.7/site-packages
Requires: 
Contributor

cdosborn commented Jul 20, 2016

Here is some sample output: (the issue linked at top contains more info).

pip show pip setuptools
Name: pip
Version: 8.2.0.dev0
Summary: The PyPA recommended tool for installing Python packages.
Home-page: https://pip.pypa.io/
Author: The pip developers
License: MIT
Location: /Users/cdosborn/proj/py/pip_resolver/pip
Requires: 
---
Name: setuptools
Version: 2.2
Summary: Easily download, build, install, upgrade, and uninstall Python packages
Home-page: https://pypi.python.org/pypi/setuptools
Author: Python Packaging Authority
License: PSF or ZPL
Location: /Users/cdosborn/.ve/a/lib/python2.7/site-packages
Requires: 
@cdosborn

This comment has been minimized.

Show comment
Hide comment
@cdosborn

cdosborn Jul 20, 2016

Contributor

I will fix the tests/docs later this evening.

What is the consensus on the fields that should be displayed? The only real culprits were Classifiers and Entry-points that made the output very long. Should we include Author email, Installer? My leaning is to have less, but @xavfernandez is advocating for more, perhaps someone else could chime in? Otherwise I will go with @xavfernandez suggestion.

Contributor

cdosborn commented Jul 20, 2016

I will fix the tests/docs later this evening.

What is the consensus on the fields that should be displayed? The only real culprits were Classifiers and Entry-points that made the output very long. Should we include Author email, Installer? My leaning is to have less, but @xavfernandez is advocating for more, perhaps someone else could chime in? Otherwise I will go with @xavfernandez suggestion.

@cdosborn

This comment has been minimized.

Show comment
Hide comment
@cdosborn

cdosborn Jul 21, 2016

Contributor

Updated output:

$ pip show pip --files
Name: pip
Version: 8.2.0.dev0
Summary: The PyPA recommended tool for installing Python packages.
Home-page: https://pip.pypa.io/
Author: The pip developers
Author-email: python-virtualenv@groups.google.com
License: MIT
Location: /Users/cdosborn/proj/py/pip_resolver/pip
Requires: 
Files:
pip show pip --verbose --files
Name: pip
Version: 8.2.0.dev0
Summary: The PyPA recommended tool for installing Python packages.
Home-page: https://pip.pypa.io/
Author: The pip developers
Author-email: python-virtualenv@groups.google.com
License: MIT
Location: /Users/cdosborn/proj/py/pip_resolver/pip
Requires: 
Metadata-Version: 1.1
Installer: 
Classifiers:
  Development Status :: 5 - Production/Stable
  Intended Audience :: Developers
  License :: OSI Approved :: MIT License
  Topic :: Software Development :: Build Tools
  Programming Language :: Python :: 2
  Programming Language :: Python :: 2.6
  Programming Language :: Python :: 2.7
  Programming Language :: Python :: 3
  Programming Language :: Python :: 3.3
  Programming Language :: Python :: 3.4
  Programming Language :: Python :: 3.5
  Programming Language :: Python :: Implementation :: PyPy
Entry-points:
  [console_scripts]
  pip = pip:main
  pip2.7 = pip:main
  pip2 = pip:main
Files:
Contributor

cdosborn commented Jul 21, 2016

Updated output:

$ pip show pip --files
Name: pip
Version: 8.2.0.dev0
Summary: The PyPA recommended tool for installing Python packages.
Home-page: https://pip.pypa.io/
Author: The pip developers
Author-email: python-virtualenv@groups.google.com
License: MIT
Location: /Users/cdosborn/proj/py/pip_resolver/pip
Requires: 
Files:
pip show pip --verbose --files
Name: pip
Version: 8.2.0.dev0
Summary: The PyPA recommended tool for installing Python packages.
Home-page: https://pip.pypa.io/
Author: The pip developers
Author-email: python-virtualenv@groups.google.com
License: MIT
Location: /Users/cdosborn/proj/py/pip_resolver/pip
Requires: 
Metadata-Version: 1.1
Installer: 
Classifiers:
  Development Status :: 5 - Production/Stable
  Intended Audience :: Developers
  License :: OSI Approved :: MIT License
  Topic :: Software Development :: Build Tools
  Programming Language :: Python :: 2
  Programming Language :: Python :: 2.6
  Programming Language :: Python :: 2.7
  Programming Language :: Python :: 3
  Programming Language :: Python :: 3.3
  Programming Language :: Python :: 3.4
  Programming Language :: Python :: 3.5
  Programming Language :: Python :: Implementation :: PyPy
Entry-points:
  [console_scripts]
  pip = pip:main
  pip2.7 = pip:main
  pip2 = pip:main
Files:
@cdosborn

This comment has been minimized.

Show comment
Hide comment
@cdosborn

cdosborn Jul 21, 2016

Contributor

Todo:

  • --files doesn't require verbose
  • no duplicate code for verbose
  • handle key missing from dict with .get(FIELD, '')
  • fix tests for python3.5
  • add tests for --verbose

I had quite a few issues running the tests: py27 was installing dev-requirements.txt but it was not sourcing it appropriately. It was throwing an ImportError on scripttest. I fixed it by doing the following in tox.ini.

 commands = py.test --timeout 300 []
-install_command = python -m pip install {opts} {packages}

-[testenv:py26]
-install_command = pip install {opts} {packages}

I was not able to get the tests to succeed for python3.5, there seemed to be some issue far away from my code.

Contributor

cdosborn commented Jul 21, 2016

Todo:

  • --files doesn't require verbose
  • no duplicate code for verbose
  • handle key missing from dict with .get(FIELD, '')
  • fix tests for python3.5
  • add tests for --verbose

I had quite a few issues running the tests: py27 was installing dev-requirements.txt but it was not sourcing it appropriately. It was throwing an ImportError on scripttest. I fixed it by doing the following in tox.ini.

 commands = py.test --timeout 300 []
-install_command = python -m pip install {opts} {packages}

-[testenv:py26]
-install_command = pip install {opts} {packages}

I was not able to get the tests to succeed for python3.5, there seemed to be some issue far away from my code.

@@ -9,9 +9,8 @@ def test_show(script):
Test end to end test for show command.
"""
result = script.pip('show', 'pip')
lines = result.stdout.split('\n')

This comment has been minimized.

@cdosborn

cdosborn Jul 21, 2016

Contributor

Prefer splitlines over split('\n')

> stdout = 'aaaa\nbbb\n'
> stdout.split('\n')
['aaaa', 'bbb', '']
> stdout.splitlines()
['aaaa', 'bbb']

The tests were counting one more than the stdout produced.

@cdosborn

cdosborn Jul 21, 2016

Contributor

Prefer splitlines over split('\n')

> stdout = 'aaaa\nbbb\n'
> stdout.split('\n')
['aaaa', 'bbb', '']
> stdout.splitlines()
['aaaa', 'bbb']

The tests were counting one more than the stdout produced.

@xavfernandez xavfernandez added this to the 8.2 milestone Jul 21, 2016

@cdosborn

This comment has been minimized.

Show comment
Hide comment
@cdosborn

cdosborn Jul 23, 2016

Contributor

@pfmoore @xavfernandez this is ready for review

Contributor

cdosborn commented Jul 23, 2016

@pfmoore @xavfernandez this is ready for review

if verbose:
logger.info("Metadata-Version: %s",
dist.get('metadata-version', ''))
logger.info("Installer: %s", dist.get('installer', ''))

This comment has been minimized.

@pfmoore

pfmoore Jul 23, 2016

Member

Not sure we need to hide Metadata-Version or Installer behind --verbose, they are only single lines after all.

@pfmoore

pfmoore Jul 23, 2016

Member

Not sure we need to hide Metadata-Version or Installer behind --verbose, they are only single lines after all.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Jul 23, 2016

Member

One minor comment, but otherwise LGTM.

There are a couple of things that are somewhat unrelated to the basic purpose of this PR (losing the initial --- and refactoring how Installer is stored) but they seem sensible, so I'm OK with them.

Member

pfmoore commented Jul 23, 2016

One minor comment, but otherwise LGTM.

There are a couple of things that are somewhat unrelated to the basic purpose of this PR (losing the initial --- and refactoring how Installer is stored) but they seem sensible, so I'm OK with them.

if dist.has_metadata('INSTALLER'):
for line in dist.get_metadata_lines('INSTALLER'):
if line.strip():
installer = line.strip()
package['installer'] = line.strip()
break

This comment has been minimized.

@cdosborn

cdosborn Jul 25, 2016

Contributor

This change is not just cosmetic. When pip show prints fields of package it does the following package.get('field', ''). If package[field] == None then None gets printed. This seemed to be the only case where a field was being assigned None rather than being undefined. (admittedly its kind of brittle)

@cdosborn

cdosborn Jul 25, 2016

Contributor

This change is not just cosmetic. When pip show prints fields of package it does the following package.get('field', ''). If package[field] == None then None gets printed. This seemed to be the only case where a field was being assigned None rather than being undefined. (admittedly its kind of brittle)

@cdosborn

This comment has been minimized.

Show comment
Hide comment
@cdosborn
Contributor

cdosborn commented Jul 25, 2016

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Jul 25, 2016

Member

Thanks. The previous version avoided that by doing if package['Installer'] is not None, but I agree your way is better.

I'd like to know what @xavfernandez thinks about making Metadata-Version and Installer depend on --verbose, otherwise I'm happy to merge this.

Member

pfmoore commented Jul 25, 2016

Thanks. The previous version avoided that by doing if package['Installer'] is not None, but I agree your way is better.

I'd like to know what @xavfernandez thinks about making Metadata-Version and Installer depend on --verbose, otherwise I'm happy to merge this.

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Jul 25, 2016

Contributor

I've no strong opinion whether Metadata-Version and Installer should be verbose or not so this looks good to me.

@cdosborn Maybe include a small changelog in https://github.com/pypa/pip/blob/master/CHANGES.txt

Contributor

xavfernandez commented Jul 25, 2016

I've no strong opinion whether Metadata-Version and Installer should be verbose or not so this looks good to me.

@cdosborn Maybe include a small changelog in https://github.com/pypa/pip/blob/master/CHANGES.txt

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Jul 25, 2016

Member

One further thought - the docs should probably include a mention of how --verbose works. Probably an extra example in docs/reference/pip_show.rst would be sufficient.

@cdosborn If you could add this and the changelog entry, I'll merge.

Member

pfmoore commented Jul 25, 2016

One further thought - the docs should probably include a mention of how --verbose works. Probably an extra example in docs/reference/pip_show.rst would be sufficient.

@cdosborn If you could add this and the changelog entry, I'll merge.

@cdosborn

This comment has been minimized.

Show comment
Hide comment
@cdosborn

cdosborn Jul 26, 2016

Contributor

Here's a small snapshot of the docs:
screen shot 2016-07-25 at 8 46 06 pm

Contributor

cdosborn commented Jul 26, 2016

Here's a small snapshot of the docs:
screen shot 2016-07-25 at 8 46 06 pm

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Jul 26, 2016

Member

One minor typo (noted in a line comment) and we're good to go.

Member

pfmoore commented Jul 26, 2016

One minor typo (noted in a line comment) and we're good to go.

@cdosborn

This comment has been minimized.

Show comment
Hide comment
@cdosborn

cdosborn Jul 26, 2016

Contributor

Welp, that's embarrassing.

screen shot 2016-07-26 at 7 14 50 am

Contributor

cdosborn commented Jul 26, 2016

Welp, that's embarrassing.

screen shot 2016-07-26 at 7 14 50 am

@cdosborn

This comment has been minimized.

Show comment
Hide comment
@cdosborn

cdosborn Jul 26, 2016

Contributor

Would you like me to rebase against current master / cleanup some of these commits?

Contributor

cdosborn commented Jul 26, 2016

Would you like me to rebase against current master / cleanup some of these commits?

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Jul 26, 2016

Member

Merge is clean, so I see no pressing need to rebase. And I don't think we have any policy of requiring commits to be squashed in PRs, so unless you'd prefer to I'm OK with it as it is.

I'll merge this later this evening - if you want to do any tidy up before then, that's fine, but up to you.

Member

pfmoore commented Jul 26, 2016

Merge is clean, so I see no pressing need to rebase. And I don't think we have any policy of requiring commits to be squashed in PRs, so unless you'd prefer to I'm OK with it as it is.

I'll merge this later this evening - if you want to do any tidy up before then, that's fine, but up to you.

@pfmoore pfmoore merged commit c7e5f52 into pypa:master Jul 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Jul 27, 2016

Member

Merged. Thanks @cdosborn for the contribution!

Member

pfmoore commented Jul 27, 2016

Merged. Thanks @cdosborn for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment