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

Get rid full file path #7898

Closed
wants to merge 11 commits into from
1 change: 1 addition & 0 deletions news/7815.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Display only the wheel filename instead of the entire filepath, when using a cached wheel.
7 changes: 5 additions & 2 deletions src/pip/_internal/operations/prepare.py
Original file line number Diff line number Diff line change
Expand Up @@ -376,8 +376,11 @@ def prepare_linked_requirement(

# TODO: Breakup into smaller functions
if link.scheme == 'file':
path = link.file_path
logger.info('Processing %s', display_path(path))
if req.from_wheel_cache:
logger.info('Processing %s (cached)', req.link.filename)
else:
logger.info('Processing %s', req.link.file_path)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this newline is needed.

else:
logger.info('Collecting %s', req.req or req)

Expand Down
12 changes: 12 additions & 0 deletions src/pip/_internal/req/req_install.py
Original file line number Diff line number Diff line change
Expand Up @@ -264,9 +264,21 @@ def populate_link(self, finder, upgrade, require_hashes):
package_name=self.name,
supported_tags=supported_tags,
)

if old_link != self.link:
logger.debug('Using cached wheel link: %s', self.link)

@property
def from_wheel_cache(self):
# type: () -> bool
"""
This function returns whether the file path is in the wheel cache.
"""
if self._wheel_cache is not None and self.link is not None:
return bool(self._wheel_cache.cache_dir in self.link.file_path)
else:
return False
Comment on lines +277 to +280
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this is safer (duck-typing) and clearer:

Suggested change
if self._wheel_cache is not None and self.link is not None:
return bool(self._wheel_cache.cache_dir in self.link.file_path)
else:
return False
try:
return self._wheel_cache.cache_dir in self.link.file_path
except AttributeError:
return False

Also please mind the docstring.

Copy link
Member

Choose a reason for hiding this comment

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

The AttributeError suggestion is unclear to me. There are too many attribute access and it’s confusing which one causes the exception. The original implementation is more explicit for me (but the bool() cast can be removed as suggested).


# Things that are valid for all kinds of requirements?
@property
def name(self):
Expand Down
57 changes: 55 additions & 2 deletions tests/unit/test_req_install.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
import os
import tempfile

import pytest
from pip._vendor.packaging.requirements import Requirement

from pip._vendor.packaging.requirements import Requirement
from pip._internal.cache import WheelCache
from pip._internal.exceptions import InstallationError
from pip._internal.models.format_control import FormatControl
from pip._internal.req.constructors import (
install_req_from_line,
install_req_from_req_string,
)
from pip._internal.req.req_install import InstallRequirement
from pip._internal.models.link import Link
from tests.lib import path_to_url
from tests.lib.wheel import make_wheel


class TestInstallRequirementBuildDirectory(object):
Expand Down Expand Up @@ -108,3 +112,52 @@ def test_install_req_from_string_with_comes_from_without_link(self):
assert install_req.link.url == wheel_url
assert install_req.req.url == wheel_url
assert install_req.is_wheel


class TestInstallRequirementWheelCache(object):

Copy link
Contributor

@McSinyx McSinyx Mar 29, 2020

Choose a reason for hiding this comment

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

Neither is this. It is, I was wrong.

def test_able_to_register_when_get_from_wheel_cache(self, tmpdir):
"""
This test to make sure that file is able to tell when a link is
from the wheel cache.
"""

Comment on lines +121 to +125
Copy link
Contributor

@McSinyx McSinyx Mar 29, 2020

Choose a reason for hiding this comment

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

What's pip policy toward PEP 257? I see this style of docstring all over functional tests. Is it time to start complying with the PEP?

format_control = FormatControl()
wheel_path = path_to_url(make_wheel(
name='pip',
version='6.9.0'
).save_to_dir(tmpdir))

wheel_cache = WheelCache(tmpdir, format_control)

install_req = install_req_from_req_string(
req_string='pip',
wheel_cache=wheel_cache
)
install_req.link = Link(wheel_path)

assert install_req.from_wheel_cache

def test_able_to_register_when_not_get_from_wheel_cache(self, tmpdir):
"""
This test to make sure that file is able to tell when a link is not
from the wheel cache.
"""

Comment on lines +143 to +147
Copy link
Contributor

Choose a reason for hiding this comment

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

So is this.

wheel_path = path_to_url(make_wheel(
name='pip',
version='6.9.0'
).save_to_dir(tmpdir))

format_control = FormatControl()

wheel_cache = WheelCache(tmpdir + '/wheel_cache', format_control)

install_req = install_req_from_req_string(
req_string='pip',
wheel_cache=wheel_cache
)

install_req.link = Link(wheel_path)

assert not install_req.from_wheel_cache