[3.8] bpo-42531: Teach importlib.resources.path to handle packages without __file__ #23611
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
|
The test failure is unrelated to this PR: |
Lib/test/test_importlib/test_path.py
Outdated
| @@ -26,6 +27,17 @@ def test_reading(self): | |||
| class PathDiskTests(PathTests, unittest.TestCase): | |||
| data = data01 | |||
|
|
|||
| def test_package_spec_origin_is_None(self): | |||
There was a problem hiding this comment.
Probably since this package represents a package not on the file system (PathDisk), it should be in a separate test case. Does the test depend at all on PathTests or PathDiskTests.data?
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @jaraco: please review the changes made to this pull request. |
python#23611 (review) This creates a package in memory and sets its __spec__.origin to None manually. Otherwise the test proceeds like the others PathTests subclasses.
c523209 to
7176b48
Compare
Indeed. This new test looks great. Thanks! |
…black formatting.
|
I started down the path of doing a minor refactor to encapsulate the "path from origin" logic, but quickly found that was a slippery slope that led me to separating the three main facets of that path function: diff --git a/Lib/importlib/resources.py b/Lib/importlib/resources.py
index 8d37d52cb8..791fd63369 100644
--- a/Lib/importlib/resources.py
+++ b/Lib/importlib/resources.py
@@ -1,3 +1,5 @@
+import contextlib
+import itertools
import os
import tempfile
@@ -182,39 +184,48 @@ def path(package: Package, resource: Resource) -> Iterator[Path]:
"""
resource = _normalize_path(resource)
package = _get_package(package)
+ results = itertools.chain(
+ _path_from_reader(package, resource),
+ _path_from_origin(package, resource),
+ _path_from_open(package, resource),
+ )
+ yield from itertools.islice(results, 1)
+
+
+def _path_from_reader(package, resource):
reader = _get_resource_reader(package)
- if reader is not None:
- try:
- yield Path(reader.resource_path(resource))
- return
- except FileNotFoundError:
- pass
- else:
+ if reader is None:
_check_location(package)
- # Fall-through for both the lack of resource_path() *and* if
- # resource_path() raises FileNotFoundError.
- file_path = None
- if package.__spec__.origin is not None:
- package_directory = Path(package.__spec__.origin).parent
- file_path = package_directory / resource
- if file_path is not None and file_path.exists():
+ return
+ with contextlib.suppress(FileNotFoundError):
+ yield Path(reader.resource_path(resource))
+
+
+def _path_from_origin(package, resource):
+ if package.__spec__.origin is None:
+ return
+ package_directory = Path(package.__spec__.origin).parent
+ file_path = package_directory / resource
+ if file_path.exists():
yield file_path
- else:
- with open_binary(package, resource) as fp:
- data = fp.read()
- # Not using tempfile.NamedTemporaryFile as it leads to deeper 'try'
- # blocks due to the need to close the temporary file to work on
- # Windows properly.
- fd, raw_path = tempfile.mkstemp()
+
+
+def _path_from_open(package, resource):
+ with open_binary(package, resource) as fp:
+ data = fp.read()
+ # Not using tempfile.NamedTemporaryFile as it leads to deeper 'try'
+ # blocks due to the need to close the temporary file to work on
+ # Windows properly.
+ fd, raw_path = tempfile.mkstemp()
+ try:
+ os.write(fd, data)
+ os.close(fd)
+ yield Path(raw_path)
+ finally:
try:
- os.write(fd, data)
- os.close(fd)
- yield Path(raw_path)
-
8000
finally:
- try:
- os.remove(raw_path)
- except FileNotFoundError:
- pass
+ os.remove(raw_path)
+ except FileNotFoundError:
+ pass
def is_resource(package: Package, name: str) -> bool:And after all of that, I'm still not happy with it (in particular the idiosyncratic use of itertools to chain iterables for input to a context manager). And while on one hand, the code reads a little better, this sort of refactor has already happened in later releases, so I think I'll just drop it, but wanted to save the work here for posterity. |
|
@wkschwartz: Status check is done, and it's a success ✅ . |
|
Thanks @wkschwartz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7. |
|
GH-24229 is a backport of this pull request to the 3.7 branch. |
…thout __file__ (pythonGH-23611) Fixes [bpo-42531]() for Python 3.8. The issue also applies to 3.7. If this PR looks like it'll be accepted, I can cherry-pick it to the 3.7 branch and submit a follow-up PR. Automerge-Triggered-By: GH:jaraco (cherry picked from commit f08c664) Co-authored-by: William Schwartz <wkschwartz@gmail.com>
Fixes bpo-42531 for Python 3.8.
The issue also applies to 3.7. If this PR looks like it'll be accepted, I can cherry-pick it to the 3.7 branch and submit a follow-up PR.
https://bugs.python.org/issue42531
Automerge-Triggered-By: GH:jaraco