bpo-32248 - Implement importlib.resources #4911

warsaw · 2017-12-17T23:16:08Z

This ports importlib_resources to the stdlib as importlib.resource. This includes documentation and tests.

I think we should do the ResourceReader implementation as a separate branch.

https://bugs.python.org/issue32248

AraHaan · 2017-12-18T01:58:21Z

Doc/whatsnew/3.7.rst

@@ -282,7 +282,14 @@ Other Language Changes
 New Modules
 ===========

-* None yet.
+importlib.resource


importlib.resources*

AraHaan · 2017-12-18T02:02:26Z

Doc/library/importlib.rst

+                     *resource* may not contain path separators and it may
+                     not have sub-resources (i.e. it cannot be a directory).
+    :type resource: ``Resource``
+    :returns: A context manager for use in a :keyword`with` statement.


shouldn't this line read :returns: A context manager for use in an :keyword:`with` statement.?

AraHaan · 2017-12-18T02:15:03Z

@warsaw I left some review comments of what I seen so far that should be corrected.

AraHaan

Alright looks good to me now.

pitrou · 2017-12-19T11:58:27Z

Doc/library/importlib.rst

+
+    Open for binary reading the *resource* within *package*.
+
+    :param package: A package name or module object.  See above for the API


This kind of markup seems unusual in sdtlib documentation.

It is unusual, so I vote to drop it.

Even I do not like this markup style in my own code and drop it.

This was copied from the standalone version, where it makes more sense and doesn't have the same consistency problems. I am rewriting it without the markup for this branch.

pitrou · 2017-12-19T11:58:47Z

Doc/library/importlib.rst

+
+The following types are defined.
+
+.. class:: Package


It looks misleading to describe it as a class, no?

Any suggestions on what directive to use?

I don't know. I think it's the first time a stdlib module exposes typing declarations. I'm not sure it's a good idea (see current python-dev discussion).

Well, typing defines types as classes, so with the lack of a better alternative, I think it's fine.

Yes... technically it's correct. What I mean is that a class is supposed to specify concrete behaviour, but being a type, Package doesn't have any methods or attributes of its own. So I don't see the point of mentioning it in the docs.

It describes the expected interfaces for arguments in the API, so it seems valuable to me. We'd still have to describe that somewhere and I don't want to have to repeat it for every method.

Yeah... It might be worth discussing on python-dev to see what a recommended practice can look like.

Ivan said that Guido prefers data:: and that makes more sense in the face of PEP 560/563. So I've made that switch in my branch for consistency. I do still think it makes sense to document them because user code may want to use those types in their own annotations.

pitrou · 2017-12-19T12:00:17Z

Doc/library/importlib.rst

+
+    Return an iterator over the contents of the package.  The iterator can
+    return resources (e.g. files) and non-resources (e.g. directories).  The
+    iterator does not recurse into subdirectories.


What is one supposed to do with directories?

It depends on how much you want to ignore the API. Basically this is an escape hatch for users if they know they are on a file system and so can actually use the subdirectory results. For others they should filter them out.

I think what's a little confusing may be that contents() returns str names (i.e. entries) so you can do with them whatever you want. Anyway, if that's what's confusing here, I'm rewriting this section a bit to make that clearer.

pitrou · 2017-12-19T12:03:03Z

Lib/importlib/resources.py

+Resource = Union[str, os.PathLike]
+
+
+def _get_package(package) -> ModuleType:


Would be nice to have docstrings on the helper functions.

pitrou · 2017-12-19T12:03:40Z

Lib/importlib/resources.py

+    full_path = os.path.join(package_path, resource)
+    try:
+        return builtins_open(full_path, mode='rb')
+    except IOError:


Just a nit, but the canonical spelling is OSError nowadays ;-)

I thought about that in the original standalone version, but forgot to make that change suggestion. I think it's worth using OSError here.

pitrou · 2017-12-19T12:05:44Z

Lib/importlib/resources.py

+            return BytesIO(data)
+
+
+def open_text(package: Package,


This is mostly the same as open_binary, so how about sharing a common implementation?

Both text and binary used to be implemented as a single function, but they got split out, since it made documenting and reasoning about the API easier. And it's actually kind of tricky to refactor as you suggest because the text versions require different calls to open() or wrapping in text wrappers. That makes the API for any refactored commonality less readable. I played around with a couple of options but wasn't super happy with them, so I didn't think it was worth it.

pitrou · 2017-12-19T12:06:36Z

Lib/importlib/resources.py

+
+def open_text(package: Package,
+              resource: Resource,
+              encoding: str = 'utf-8',


What does PEP 8 say about this? I would expect no spaces around the equals sign (i.e. encoding: str='utf-8').

I don't think PEP 8 says anything about type hints. This is just the style based on the type hint PEPs.

I see. But if we take the advice for default parameter values, then there should be no space around the equals :-)

Yep, it's one of those situations where I'm not sure which way to go, so I just went with the PEP that Guido wrote most recently. 😁

I found the extra spaces a little jarring to originally, but trying it both ways, I think I like the spaces actually. Type hints make default arguments read differently so I don't like the stronger association to str than encoding that the lack of spaces implies. It would be interesting to have that discussion on PEP 8!

pitrou · 2017-12-19T12:07:17Z

Lib/importlib/resources.py

+
+
+@contextmanager
+def path(package: Package, resource: Resource) -> Iterator[Path]:


Why does it return an iterator? Is it the standard way to express context managers?

Because the contextmanager decorator transforms the function to return a context manager. But the undecorated function itself still only yields Path instances. Basically it looks weird in source but at least mypy does the right thing.

I think this issue tries to get at better syntax for describing things like @contextmanager s and such.

pitrou · 2017-12-19T12:08:58Z

Lib/importlib/resources.py

+    # contents, and make sure that this entry doesn't have sub-entries.
+    archive_path = package.__spec__.loader.archive   # type: ignore
+    package_directory = Path(package.__spec__.origin).parent
+    with ZipFile(archive_path) as zf:


I'm extremely surprised to see ZIpFile-specific code here. Isn't this whole module supposed to be an abstraction above the various concrete loaders? This seems wrong to me.

This is until we get proper support in zipimport itself (which has not happened yet). It's also an accidental hold-over since the PyPI project has to support back to Python 3.4 and 2.7.

Right. When we get built-in zipimport support, we can drop this code (and the in-stdlib version doesn't need to worry about Pythons < 3.7).

pitrou · 2017-12-19T12:09:11Z

Lib/importlib/resources.py

+        if archive_path is None:
+            raise
+        relpath = package_directory.relative_to(archive_path)
+        with ZipFile(archive_path) as zf:


brettcannon

I think @pitrou has some legitimate change requests.

brettcannon · 2017-12-19T21:47:09Z

Doc/whatsnew/3.7.rst

@@ -282,7 +282,14 @@ Other Language Changes
 New Modules
 ===========

-* None yet.
+importlib.resources


Can we use :mod:`importlib.resources` in titles so it links to the documentation?

None of the other entries in this section (i.e. the "Improved Modules") uses links, so I just went with consistency here.

bedevere-bot · 2017-12-19T21:48:51Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

warsaw · 2017-12-27T03:27:36Z

I have made the requested changes; please review again

bedevere-bot · 2017-12-27T03:27:38Z

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

merwok · 2017-12-29T03:24:12Z

Doc/library/importlib.rst

+---------------------------------------
+
+.. module:: importlib.resources
+    :synopsis: Package resource reading, opening, and access


Block contents in reST directives should start with three spaces, not four.

Seems like we have some inconsistency in this file. I just copied it from every other section in that file except the top level .. module:: importlib.

merwok · 2017-12-29T03:27:04Z

Lib/importlib/resources.py

+    if reader is not None:
+        return TextIOWrapper(reader.open_resource(resource), encoding, errors)
+    # Using pathlib doesn't work well here due to the lack of 'strict'
+    # argument for pathlib.Path.resolve() prior to Python 3.6.


A later comment mentions that this version of the code (I mean the stdlib version in comparison to the PyPI backport) can be compatible with 3.7+ only.

Thanks. I'll fix this by actually using Path objects in the stdlib version (and removing the comment).

brettcannon

Couple minor things to make decisions on that will deviate the code even further from the PyPI code in minor ways.

brettcannon · 2017-12-29T20:43:15Z

Lib/importlib/resources.py

+from io import BytesIO, TextIOWrapper
+from pathlib import Path
+from types import ModuleType
+from typing import Iterator, Optional, Set, Union   # noqa: F401


Should we drop the directive comment?

Or better yet, do we even need the typing-related imports with the "lazy" type hint changes in Python 3.7?

Has that landed yet? I tried removing them (except for Union, which we'd still need because of the Package and Resource definitions). Python was not happy. So I guess we can address that again later.

I could remove the #noqa directive, but it makes my editor happier, so I think I'll leave them in for now.

brettcannon · 2017-12-29T20:47:05Z

Lib/importlib/resources.py

+
+
+def is_resource(package: Package, name: str) -> bool:
+    """True if `name` is a resource inside `package`.


'name', 'package' based on what I think of as common practice for marking common-word parameter names in docstrings.

Sure. Consistency within the project is probably useful, though I've tended to use backticks (i.e. markdown) for these in my own code.

brettcannon · 2017-12-29T20:48:17Z

Lib/importlib/resources.py

+        toc = zf.namelist()
+    relpath = package_directory.relative_to(archive_path)
+    candidate_path = relpath / name
+    for entry in toc:                               # pragma: nobranch


Drop the pragma?

I suppose we'll never run coverage over this code inside the stdlib, so I'm okay with that. Plus this code will get replaced once we implement ResourceReader in zipimport.

brettcannon · 2017-12-29T20:48:35Z

Lib/importlib/resources.py

+            subparts = path.parts[len(relpath.parts):]
+            if len(subparts) == 1:
+                yield subparts[0]
+            elif len(subparts) > 1:                 # pragma: nobranch


warsaw · 2017-12-29T23:23:37Z

Looks like Windows is unhappy.

This reverts commit 3607846. Let's see if this is the cause of the Windows failures.

warsaw added 7 commits December 17, 2017 14:19

Port importlib_resources to importlib.resources

19fab84

Port importlib_resources to importlib.resources

ebed26a

Merge branch 'issue32248' of github.com:warsaw/cpython into issue32248

e8e84f4

Update blurb

7ffc7dc

Port the documentation

0da9744

Add a What's New for importlib.resources

871367d

Clean up

d41bac3

warsaw requested a review from a team December 17, 2017 23:16

the-knights-who-say-ni added the CLA signed label Dec 17, 2017

bedevere-bot added the awaiting merge label Dec 17, 2017

AraHaan suggested changes Dec 18, 2017

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting merge labels Dec 18, 2017

AraHaan suggested changes Dec 18, 2017

View reviewed changes

warsaw added 2 commits December 18, 2017 13:27

Fix two typos

d7ec339

Fix another typo from a previous branch

08859e2

warsaw mentioned this pull request Dec 18, 2017

bpo-32248: Implement importlib.abc.ResourceReader #4892

Merged

AraHaan approved these changes Dec 18, 2017

View reviewed changes

pitrou reviewed Dec 19, 2017

View reviewed changes

brettcannon requested changes Dec 19, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Dec 19, 2017

warsaw added 2 commits December 26, 2017 18:52

Merge branch 'master' into issue32248

0fcc0c9

Respond to review

05353ed

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 27, 2017

warsaw added 2 commits December 28, 2017 10:14

Merge branch 'master' into issue32248

4085c9c

Use data:: for types as per typing.rst

e143941

merwok reviewed Dec 29, 2017

View reviewed changes

warsaw added 2 commits December 29, 2017 10:11

Merge branch 'master' into issue32248

cc28abb

This is 3.7 so we can use Path.resolve()

3607846

brettcannon approved these changes Dec 29, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Dec 29, 2017

A couple of last minute review comments

1205e2a

warsaw added 2 commits December 30, 2017 10:52

Revert "This is 3.7 so we can use Path.resolve()"

1a68c68

This reverts commit 3607846. Let's see if this is the cause of the Windows failures.

Remove some obsolete comments

535da74

warsaw merged commit deae6b4 into python:master Dec 30, 2017

bedevere-bot removed the awaiting merge label Dec 30, 2017

warsaw deleted the issue32248 branch December 30, 2017 20:18

jaraco mentioned this pull request Oct 21, 2020

Sync with stdlib changes in GH#4911 - [merged] python/importlib_resources#140

Closed


		Open for binary reading the resource within package.

		:param package: A package name or module object. See above for the API

		Resource = Union[str, os.PathLike]


		def _get_package(package) -> ModuleType:



		@contextmanager
		def path(package: Package, resource: Resource) -> Iterator[Path]:



		def is_resource(package: Package, name: str) -> bool:
		"""True if `name` is a resource inside `package`.

bpo-32248 - Implement importlib.resources #4911

bpo-32248 - Implement importlib.resources #4911

Conversation

warsaw commented Dec 17, 2017 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

AraHaan Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

AraHaan commented Dec 18, 2017

AraHaan 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

warsaw Dec 27, 2017 • edited Loading

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

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

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

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

warsaw Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettcannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Dec 19, 2017

warsaw commented Dec 27, 2017

bedevere-bot commented Dec 27, 2017

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

brettcannon 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warsaw commented Dec 29, 2017

warsaw commented Dec 17, 2017 •

edited by bedevere-bot

Loading

AraHaan Dec 18, 2017 •

edited

Loading

warsaw Dec 27, 2017 •

edited

Loading

warsaw Dec 27, 2017 •

edited

Loading