Created new module: unpacking #6866

ivenk · 2019-08-12T23:22:24Z

Only code refactoring was done.

Moved download::unpack_file_url() and misc::unzip_file(), misc::untar_file(), misc::unpack_file() to a new module. The new module is named unpacking and is located in pip/_internal/utils/

I ran all the tests and locally everything works.

…ions

cjerdonek

Thanks! Some initial comments.

cjerdonek · 2019-08-12T23:35:23Z

src/pip/_internal/download.py

@@ -21,23 +21,19 @@
 from pip._vendor.requests.models import CONTENT_CHUNK_SIZE, Response
 from pip._vendor.requests.structures import CaseInsensitiveDict
 from pip._vendor.requests.utils import get_netrc_auth
-# NOTE: XMLRPC Client is not annotated in typeshed as on 2017-07-17, which is


This comment shouldn't be removed.

cjerdonek · 2019-08-12T23:35:48Z

src/pip/_internal/download.py

 from pip._vendor.six.moves import xmlrpc_client  # type: ignore
 from pip._vendor.six.moves.urllib import parse as urllib_parse
 from pip._vendor.six.moves.urllib import request as urllib_request

 import pip
 from pip._internal.exceptions import HashMismatch, InstallationError
 from pip._internal.models.index import PyPI
-# Import ssl from compat so the initial import occurs in only one place.


This comment also shouldn't be removed (don't remove comments unrelated to the PR).

cjerdonek · 2019-08-12T23:36:31Z

src/pip/_internal/utils/misc.py

 from collections import deque

 from pip._vendor import pkg_resources
-# NOTE: retrying is not annotated in typeshed as on 2017-07-17, which is


cjerdonek · 2019-08-12T23:40:39Z

src/pip/_internal/utils/unpacking.py

+    current_umask,
+    ensure_dir,
+    file_contents,
+    has_leading_dir,


is_svn_page(), has_leading_dir() and split_leading_dir() should also be moved to the new module, as the only place they're used is in the unpack functions.

cjerdonek · 2019-08-12T23:43:53Z

src/pip/_internal/utils/unpacking.py

+    logger.debug('lzma module is not available')
+
+
+def unpack_file_url(


It would be best if the functions in the module are ordered so that functions only depend on function that occur above them in the module, so ordered low-level to high-level. That way the module can be read top-to-bottom, and it becomes easier to locate things.

Also, the tests of the functions moved to this module should be moved to e.g. test_unpacking.py, following the pattern of other test modules.

cjerdonek · 2019-08-12T23:50:19Z

src/pip/_internal/utils/unpacking.py

+        is_dir_url,
+        url_to_path,
+        _check_download_dir,
+        _copy_file


Also, this module shouldn't have dependencies on download.py (avoid introducing new nested imports, by the way). Rather, the functions in download.py that unpack_file_url() depends on should also be moved into this module. We don't want to have dependencies from utils to higher-level modules outside of it like download.py.

chrahunt

A few small comments, but in general I'm concerned about the placement of unpack_file_url. It embeds a lot of really specific business logic whereas the others are closer to what I'd call utilities. Maybe it would be worth considering keeping it outside utils.

chrahunt · 2019-08-12T23:50:33Z

src/pip/_internal/utils/misc.py

-        Any, AnyStr, Container, Iterable, List, Mapping, Match, Optional, Text,
-        Tuple, Union, cast,
+        Any, Container, Iterable, List, Mapping, Optional, Text,
+        Tuple, cast, AnyStr, Union, Match


I would alphabetize these.

chrahunt · 2019-08-12T23:57:41Z

src/pip/_internal/utils/unpacking.py

+    from typing import Optional
+    from pip._internal.models.link import Link
+    from pip._internal.utils.hashes import Hashes
+


I would put two lines between the end of imports and body.

chrahunt · 2019-08-12T23:58:52Z

src/pip/_internal/utils/unpacking.py

@@ -0,0 +1,286 @@
+# mypy: strict-optional=False


Elsewhere we pair this with # The following comment should be removed at some point in the future., we should do the same here.

cjerdonek · 2019-08-13T01:06:02Z

Maybe it would be worth considering keeping it outside utils.

What location are you suggesting for unpack_file_url()? A stand-alone module alongside wheel.py or in wheel.py itself? I don't think it should be inside download.py. I wouldn't object to putting it in wheel.py. One of the main things IMO is removing the dependency of wheel.py on download.py. I do think that putting it in unpacking.py is better than the status quo from a dependency perspective. And we could always continue with further improvements afterwards.

cjerdonek · 2019-08-13T01:12:50Z

I wouldn't object to putting it in wheel.py.

Actually, now that I look at it, wheel.py is also a file with ~1200 lines, so I don't think we'd want to be complicating that file further. So a stand-alone file outside of utils? A stand-alone file inside operations? We can always revisit this further after the PR. One thing consistent with putting it in utils is that it doesn't add additional third-party dependencies to utils (like putting network-accessing code would). These are functions "without significant third-party dependencies."

chrahunt · 2019-08-13T01:15:02Z

I agree, it shouldn't be in download.py. A standalone module would be better than wheel.py IMO since we can iterate with less tedious import management (and like you mention, it's already pretty big). Maybe local_project.py? If a few minutes of thought doesn't result in a clear winner then I'm OK with it going into unpacking.py, just keeping in mind that we'll need to drag a few other things into it as a result of #6844.

cjerdonek · 2019-08-13T01:26:04Z

What about something like unpacker.py, to give it a file name different from unpacking to make grepping easier? The idea is that this module supports unpacking for a specific use case, as it contains business logic as you say.

chrahunt · 2019-08-13T01:29:12Z

Sounds fine to me. In _internal?

cjerdonek · 2019-08-13T01:29:27Z

Yes, sibling to download.py and wheel.py.

cjerdonek · 2019-08-13T01:36:09Z

Okay, great! @ivenk Did you get that? You can continue with your unpacking.py changes and review comments, but in addition put unpack_file_url() and the download.py functions it depends on in a second new module called unpacker.py next to download.py.

ivenk · 2019-08-13T18:20:03Z

@cjerdonek i got it, thanks ! I will fix everything mentioned here and create a new file for unpack_file_url() and required functions.

…o unpacker.py

BrownTruck · 2019-08-16T00:00:06Z

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!

cjerdonek · 2019-08-19T09:14:50Z

@ivenk This needs a rebase. Also, in your latest PR, it looks like all of the from pip._vendor imports were moved to be after the from pip._internal imports (e.g. in download.py). You should keep the import statements in their same relative location, i.e. don't do any reordering of import statements.

ivenk · 2019-08-19T15:22:08Z

@cjerdonek thanks for the hint i think that happened during some refactoring from the ide.
Unfortunately i do not have time to look at this until the weekend so if anyone wants to pick this up, feel free to do so.

cjerdonek · 2019-08-20T01:31:16Z

Okay, thanks for letting us know. Maybe we'll switch back to proceeding on some things that we were holding off on for this, or else take this (or parts of this) up to help ease the burden.

chrahunt · 2019-09-21T21:06:52Z

Hi @ivenk. We were able to take care of this in #7028, #7037, and #7044, which were definitely easier having seen the changes and comments here. Thanks a lot for your help. :)

I will close this now, but please don't hesitate to pick up any other issues you find interesting.

iven_k added 7 commits August 11, 2019 22:39

Fixes pypa#6861. Created new module:unpacking and moved related funct…

17f7a82

…ions

Fixed most mypy issues

3f13c88

Fixed all mypy issues

cabc15e

Fixed unit test for py3. Broke up circular dependency

aa6ddc8

Fixed unit test issues

6069a49

fixed linting issues

d5f6173

Added news trivial news file

8c855fa

cjerdonek reviewed Aug 12, 2019

View reviewed changes

chrahunt reviewed Aug 13, 2019

View reviewed changes

cjerdonek mentioned this pull request Aug 14, 2019

Ignore errors copying socket files for source installs (in Python 3). #6844

Merged

iven_k added 7 commits August 14, 2019 17:09

restored the comments

4ff973d

Created new module unpacker and moved unpack_file_url() to it

a565b35

Moved all functions required for unpack_file_url() from download.py t…

27f55d1

…o unpacker.py

ordered functions. alphabetically ordered mypy imports

3403839

Moved helper functions from misc to unpacking

47e1a9d

formating

2c8083a

Fixed mypy import issues

e994941

BrownTruck added the needs rebase or merge PR has conflicts with current master label Aug 16, 2019

chrahunt mentioned this pull request Sep 18, 2019

Move unpacking functions from utils.misc to utils.unpacking. #7028

Merged

chrahunt closed this Sep 21, 2019

lock bot added the auto-locked Outdated issues that have been locked by automation label Oct 21, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Created new module: unpacking #6866

Created new module: unpacking #6866

ivenk commented Aug 12, 2019

cjerdonek left a comment

cjerdonek Aug 12, 2019

cjerdonek Aug 12, 2019

cjerdonek Aug 12, 2019

cjerdonek Aug 12, 2019

cjerdonek Aug 12, 2019

cjerdonek Aug 12, 2019

chrahunt left a comment

chrahunt Aug 12, 2019

chrahunt Aug 12, 2019

chrahunt Aug 12, 2019

cjerdonek commented Aug 13, 2019

cjerdonek commented Aug 13, 2019

chrahunt commented Aug 13, 2019 •

edited

Loading

cjerdonek commented Aug 13, 2019 •

edited

Loading

chrahunt commented Aug 13, 2019

cjerdonek commented Aug 13, 2019 •

edited

Loading

cjerdonek commented Aug 13, 2019 •

edited

Loading

ivenk commented Aug 13, 2019

BrownTruck commented Aug 16, 2019

cjerdonek commented Aug 19, 2019

ivenk commented Aug 19, 2019

cjerdonek commented Aug 20, 2019

chrahunt commented Sep 21, 2019

		logger.debug('lzma module is not available')


		def unpack_file_url(

Created new module: unpacking #6866

Created new module: unpacking #6866

Conversation

ivenk commented Aug 12, 2019

cjerdonek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrahunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjerdonek commented Aug 13, 2019

cjerdonek commented Aug 13, 2019

chrahunt commented Aug 13, 2019 • edited Loading

cjerdonek commented Aug 13, 2019 • edited Loading

chrahunt commented Aug 13, 2019

cjerdonek commented Aug 13, 2019 • edited Loading

cjerdonek commented Aug 13, 2019 • edited Loading

ivenk commented Aug 13, 2019

BrownTruck commented Aug 16, 2019

cjerdonek commented Aug 19, 2019

ivenk commented Aug 19, 2019

cjerdonek commented Aug 20, 2019

chrahunt commented Sep 21, 2019

chrahunt commented Aug 13, 2019 •

edited

Loading

cjerdonek commented Aug 13, 2019 •

edited

Loading

cjerdonek commented Aug 13, 2019 •

edited

Loading

cjerdonek commented Aug 13, 2019 •

edited

Loading