Get rid full file path #7898

microcat49 · 2020-03-25T14:58:14Z

Fixes and closes #7815

This change will switch from using full file path for wheel to using just the name of the package.

…ad of the full file path.

pfmoore

Also, this should include tests.

src/pip/_internal/operations/prepare.py

pradyunsg · 2020-03-26T13:44:05Z

Thanks for the PR @microcat49! If you could respond to the review comments when you have the time to, that'd be great! :)

microcat49 · 2020-03-26T15:37:57Z

Hey @pradyunsg sorry about the delayed response I'm still looking into how I could fix the issue raised in the review comment.

pradyunsg · 2020-03-26T18:21:10Z

@microcat49 thanks for the quick response! No hurries. Feel free to ask follow up questions if you have any. :)

pradyunsg

A few comments and this definitely needs tests.

src/pip/_internal/operations/prepare.py

src/pip/_internal/req/req_install.py

news/7815.bugfix

… the link. Also adds unit tests.

McSinyx

Some nitpicking, please don't take me too seriously though!

McSinyx · 2020-03-29T05:18:43Z

src/pip/_internal/operations/prepare.py

+                logger.info('Processing %s (cached)', req.link.filename)
+            else:
+                logger.info('Processing %s', req.link.file_path)
+


I don't think this newline is needed.

McSinyx · 2020-03-29T05:19:30Z

tests/unit/test_req_install.py

+
+
+class TestInstallRequirementWheelCache(object):
+


~~Neither is this.~~ It is, I was wrong.

McSinyx · 2020-03-29T05:20:29Z

tests/unit/test_req_install.py

+        """
+        This test to make sure that file is able to tell when a link is
+        from the wheel cache.
+        """
+


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?

McSinyx · 2020-03-29T05:20:41Z

tests/unit/test_req_install.py

+        """
+        This test to make sure that file is able to tell when a link is not
+        from the wheel cache.
+        """
+


So is this.

McSinyx · 2020-03-29T05:25:17Z

src/pip/_internal/req/req_install.py

+        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


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.

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).

sbidoul · 2020-03-29T10:05:25Z

I am not convinced it is worthwhile to continue this PR without a preliminary refactoring.
See #7815 (comment) and following comments.
Also note #7612 already has some refactoring of the caching part in populate_link.

microcat49 · 2020-03-30T15:48:50Z

Hey @uranusjr @sbidoul @McSinyx Please let me know if you'd like me to move forward with this PR? I'm happy to continue it just seems like there is disagreements whether this PR should go through. I wanted to check to make sure before I continued?

BrownTruck · 2020-03-31T16:15:05Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

pradyunsg · 2021-02-26T10:41:13Z

Closing due to lack of movement here. Please feel free to file a new PR if you want to get the ball rolling again! :)

microcat49 added 3 commits March 25, 2020 10:23

This commit changest the logging to use the name of the package inste…

bc7124e

…ad of the full file path.

add news file

3450e5b

add new line

0c7c877

pfmoore requested changes Mar 25, 2020

View reviewed changes

src/pip/_internal/operations/prepare.py Outdated Show resolved Hide resolved

pradyunsg added S: awaiting response Waiting for a response/more information type: enhancement Improvements to functionality labels Mar 26, 2020

check to see if it is from the wheel cache

764fca3

pradyunsg reviewed Mar 27, 2020

View reviewed changes

src/pip/_internal/operations/prepare.py Outdated Show resolved Hide resolved

src/pip/_internal/req/req_install.py Outdated Show resolved Hide resolved

news/7815.bugfix Outdated Show resolved Hide resolved

microcat49 added 4 commits March 28, 2020 22:00

Switches the variable is_from_wheel_cache to a function that looks at…

a611a84

… the link. Also adds unit tests.

gets rid of empyt space.

f79c530

Fixing linting errors

e33c22a

linting fixes

b5e1a88

McSinyx reviewed Mar 29, 2020

View reviewed changes

microcat49 added 3 commits March 29, 2020 10:20

lint changes

3449d5f

lint changes

7eb1af5

lint changes

a8006e2

uranusjr mentioned this pull request Mar 30, 2020

Move wheel cache out of InstallRequirement #7801

Merged

BrownTruck added the needs rebase or merge PR has conflicts with current master label Mar 31, 2020

pradyunsg closed this Feb 26, 2021

uranusjr mentioned this pull request Mar 7, 2021

When reporting candidate location, handle cached wheels better #8463

Open

github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid full file path #7898

Get rid full file path #7898

microcat49 commented Mar 25, 2020 •

edited

Loading

pfmoore left a comment

pradyunsg commented Mar 26, 2020

microcat49 commented Mar 26, 2020

pradyunsg commented Mar 26, 2020

pradyunsg left a comment

McSinyx left a comment

McSinyx Mar 29, 2020

McSinyx Mar 29, 2020 •

edited

Loading

McSinyx Mar 29, 2020 •

edited

Loading

McSinyx Mar 29, 2020

McSinyx Mar 29, 2020

uranusjr Mar 29, 2020

sbidoul commented Mar 29, 2020

microcat49 commented Mar 30, 2020

BrownTruck commented Mar 31, 2020

pradyunsg commented Feb 26, 2021



		class TestInstallRequirementWheelCache(object):

Get rid full file path #7898

Get rid full file path #7898

Conversation

microcat49 commented Mar 25, 2020 • edited Loading

pfmoore left a comment

Choose a reason for hiding this comment

pradyunsg commented Mar 26, 2020

microcat49 commented Mar 26, 2020

pradyunsg commented Mar 26, 2020

pradyunsg left a comment

Choose a reason for hiding this comment

McSinyx left a comment

Choose a reason for hiding this comment

McSinyx Mar 29, 2020

Choose a reason for hiding this comment

McSinyx Mar 29, 2020 • edited Loading

Choose a reason for hiding this comment

McSinyx Mar 29, 2020 • edited Loading

Choose a reason for hiding this comment

McSinyx Mar 29, 2020

Choose a reason for hiding this comment

McSinyx Mar 29, 2020

Choose a reason for hiding this comment

uranusjr Mar 29, 2020

Choose a reason for hiding this comment

sbidoul commented Mar 29, 2020

microcat49 commented Mar 30, 2020

BrownTruck commented Mar 31, 2020

pradyunsg commented Feb 26, 2021

microcat49 commented Mar 25, 2020 •

edited

Loading

McSinyx Mar 29, 2020 •

edited

Loading

McSinyx Mar 29, 2020 •

edited

Loading