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

Added clearer error reporting when skipping pre-releases #655

Merged
merged 2 commits into from
Apr 26, 2018

Conversation

wolph
Copy link
Contributor

@wolph wolph commented Apr 25, 2018

Added clearer error reporting when skipping pre-releases by splitting the output by regular/pre-release versions.

@wolph wolph requested a review from vphilippon April 25, 2018 01:48
@wolph
Copy link
Contributor Author

wolph commented Apr 25, 2018

This pull request is to fix: pypa/pipenv#2046

Copy link
Member

@vphilippon vphilippon left a comment

Choose a reason for hiding this comment

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

The overall idea is good, I like it 👍

I've put some things to address, and that should be ok after that.

@@ -9,14 +9,31 @@ def __init__(self, ireq, candidates_tried, index_urls):
self.index_urls = index_urls

def __str__(self):
sorted_versions = sorted(c.version for c in self.candidates_tried)
sorted_candidates = sorted(self.candidates_tried)
Copy link
Member

Choose a reason for hiding this comment

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

sorted_candidates is unused, that's what makes flake8 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.

You're right, that's a small oversight from me. Initially I was using it but dropped it in favour of an explicit loop instead of a few list comprehensions.

lines.append('Tried: {}'.format(', '.join(versions)))

if pre_versions:
lines.append('Skipped pre-versions: {}'.format(', '.join(pre_versions)))
Copy link
Member

Choose a reason for hiding this comment

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

That statement isn't true in all cases: if the user did provide --pre, then the pre-release versions where actually "tried".
Incompatible requirements can still occur with --pre (ex: mypackage<1.2.3a1,>=1.2.3a2), so we still want to give the proper message.

From the top of my head, I'd say we could pass the PackageFinder instance (self.finder) through NoCandidateFound.__init__ instead of index_urls, and use the info from the finder instance to make the proper message (getting the index URLs and check allow_all_prereleases).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually thought it wouldn't occur because in that case it would match, but that's obviously not always the case... I'll see what I can do about it :)

@wolph wolph dismissed vphilippon’s stale review April 26, 2018 09:29

I've fixed all of the issues and added tests :)

Copy link
Member

@vphilippon vphilippon left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@vphilippon vphilippon added this to the 2.1.0 milestone Apr 26, 2018
@wolph wolph merged commit 8d8d4ea into master Apr 26, 2018
@wolph wolph deleted the pre-release-dependency-warning branch April 26, 2018 15:58
@vphilippon vphilippon modified the milestones: 2.1.0, 2.0.2 Apr 28, 2018
suutari-ai pushed a commit to suutari/prequ that referenced this pull request May 21, 2018
…-integration

* 'master' of github.com:jazzband/pip-tools:
  Add comment, fix tests
  Update for compatibility with pip 8-10
  Drop pip 9 vendor dir
  Added 2.0.2 changelog (after actual release).
  Added clearer error reporting when skipping pre-releases (jazzband#655)
  Added a missing (important) changelog entry in 2.0.0
  Updated changelog for 2.0.1 (after release)
  Add missing package_data from vendored pip 9.0.3
  Changelog for v2.0.0
  Change a misleading example about version pinning
  Filter pip environment markers
  Remove setuptools dependency
  Remove the pip version "dimension" from the test matrix
  Remove obsolete Makefile - Using Jazzband release process
  Update and add mention in README.rst about pip vendoring
  Omited piptools/_vendored and piptools/_compat from tests
  Remove now obsolete pip version check due to vendoring
  Add piptools/vendored to sys.path on start
  Add pipools/_vendored/README.rst
  Add vendored pip 9.0.3
  Take CLI parameter as --user, pass through to pip install
  Option to restrict attention to user directory
  cache hashes for --generate-hashes
  Fix capitalization: PyPi -> PyPI
  Pass python_requires argument to setuptools
  Add dev-requirements.txt file to install packages for testing
suutari-ai pushed a commit to suutari/prequ that referenced this pull request May 21, 2018
* pip-tools-integration:
  Add comment, fix tests
  Update for compatibility with pip 8-10
  Drop pip 9 vendor dir
  Added 2.0.2 changelog (after actual release).
  Added clearer error reporting when skipping pre-releases (jazzband#655)
  Added a missing (important) changelog entry in 2.0.0
  Updated changelog for 2.0.1 (after release)
  Add missing package_data from vendored pip 9.0.3
  Changelog for v2.0.0
  Change a misleading example about version pinning
  Filter pip environment markers
  Remove setuptools dependency
  Remove the pip version "dimension" from the test matrix
  Remove obsolete Makefile - Using Jazzband release process
  Update and add mention in README.rst about pip vendoring
  Omited piptools/_vendored and piptools/_compat from tests
  Remove now obsolete pip version check due to vendoring
  Add piptools/vendored to sys.path on start
  Add pipools/_vendored/README.rst
  Add vendored pip 9.0.3
  Take CLI parameter as --user, pass through to pip install
  Option to restrict attention to user directory
  cache hashes for --generate-hashes
  Fix capitalization: PyPi -> PyPI
  Pass python_requires argument to setuptools
  Add dev-requirements.txt file to install packages for testing
suutari-ai pushed a commit to suutari/prequ that referenced this pull request May 21, 2018
* pip10:
  tests: Remove print statement
  AppVeyour: Fix pip upgrade command
  PyPIRepository: Use PIP_10_OR_NEWER var rather than exception
  tests: Move Pip version comparison to _pip_compat
  Remove dev-requirements.txt
  requirements-dev: Add "-e ." for install Prequ itself
  requirements: Annotate dependencies
  requirements-*.txt: Update requirements
  Tox,AppVeyor: Test with pip10 rather than piplatest
  get_pinned_version: Fix constructing of InstallRequirement
  cache: Fix importing and constructing of Requirement
  tests: Fix hash generating tests
  tests: Fix NoCandidateFound exception tests
  tests: Fix assertion of generated header
  PyPIRepository: Clean-up wheel cache when done
  PyPIRepository: Remove package itself from its dependencies
  PyPIRepository: Remove unneeded build dir clean-up code
  tests: Fix platform respecting test
  tests: Patch correct pep425tags on Pip 10
  _pip_compat: Add variable PIP_10_OR_NEWER
  Move WheelCache import to _pip_compat
  Fix pip imports
  Rewrite _pip_compat to simpler form
  tests: Do not read pip version from env
  Remove unused temp_environ function
  Fix style issues and unused imports
  Add comment, fix tests
  Update for compatibility with pip 8-10
  Drop pip 9 vendor dir
  Added 2.0.2 changelog (after actual release).
  Added clearer error reporting when skipping pre-releases (jazzband#655)
  Added a missing (important) changelog entry in 2.0.0
  Updated changelog for 2.0.1 (after release)
  Add missing package_data from vendored pip 9.0.3
  Changelog for v2.0.0
  Change a misleading example about version pinning
  Filter pip environment markers
  Remove setuptools dependency
  Remove the pip version "dimension" from the test matrix
  Remove obsolete Makefile - Using Jazzband release process
  Update and add mention in README.rst about pip vendoring
  Omited piptools/_vendored and piptools/_compat from tests
  Remove now obsolete pip version check due to vendoring
  Add piptools/vendored to sys.path on start
  Add pipools/_vendored/README.rst
  Add vendored pip 9.0.3
  Take CLI parameter as --user, pass through to pip install
  Option to restrict attention to user directory
  cache hashes for --generate-hashes
  Fix capitalization: PyPi -> PyPI
  Pass python_requires argument to setuptools
  Add dev-requirements.txt file to install packages for testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants