bpo-32248: Implement importlib.abc.ResourceReader by brettcannon · Pull Request #4892 · python/cpython

brettcannon · 2017-12-15T18:42:16Z

https://bugs.python.org/issue32248

Nothing more than a copy of the docstrings.

warsaw

You might want to try to lift as much of the text from the standalone package's description of the ABC

warsaw · 2017-12-15T19:57:33Z

Doc/library/importlib.rst

+.. class:: ResourceReader
+
+    An :term:`abstract base class` for :term:`loaders <loader>`
+    representing a :term:`package` to provide the ability to read


This reads a bit awkwardly for me. What about "An abstract base class for package loaders which provide the ability to read resources"?

warsaw · 2017-12-15T19:59:08Z

Doc/library/importlib.rst

+
+    From the perspective of this class, a *resource* is a binary
+    artifact that is shipped within the package that this loader
+    represents. Typically this is something like a data file that


That's another awkward turn of a similar phrase. "package that this loader represents" seems weird to me ;)

warsaw · 2017-12-15T20:01:47Z

Doc/library/importlib.rst

+    representing a :term:`package` to provide the ability to read
+    *resources*.
+
+    From the perspective of this class, a *resource* is a binary


Should it say "from the perspective of this ABC"?

warsaw · 2017-12-15T20:04:08Z

Doc/library/importlib.rst

+        If the resource does not concretely exist on the file system,
+        raise :exc:`FileNotFoundError`.
+
+    .. abstractmethod:: is_resource(path)


In the standalone package I called this name so that it doesn't necessarily imply a path (which could include slashes).

warsaw · 2017-12-15T20:04:51Z

Doc/library/importlib.rst

+    .. abstractmethod:: contents()
+
+        Returns an :term:`iterator` of strings over the contents of
+        the package. Due note that it is not required that all names


warsaw · 2017-12-15T20:06:45Z

Lib/test/test_importlib/test_abc.py

+
+class ResourceReaderDefaultsTests(ABCTestHarness):
+
+    SPLIT = make_abc_subclasses(ResourceReader)


Is SPLIT required by ABCTestHarness? It seems an odd choice of terms.

Yes and I didn't name it.

bedevere-bot · 2017-12-15T20:07:37Z

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

brettcannon · 2017-12-15T23:31:54Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-12-15T23:31:55Z

Nobody expects the Spanish Inquisition!

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

warsaw · 2017-12-15T23:35:56Z

LGTM, thanks @brettcannon !

If you close the bug, can you open a new one for the implementation? I'm also cool with keeping the original bug open for the ABC implementation branch. 'Sup to you!

brettcannon · 2017-12-15T23:41:10Z

I say just keep the issue open. We can just expand on the NEWS entry as necessary.

pitrou · 2017-12-18T10:14:58Z

Doc/library/importlib.rst

+    versus on the file system.
+
+    For any of methods of this class, a *resource* argument is
+    expected to be a :term:`file-like object` which represents


So open_resource takes a file object and returns a file object? I may be misunderstanding here...

I think that term should say that a resource is a path-like object. I'll fix that in my follow up branch.

pitrou · 2017-12-18T10:15:52Z

Doc/library/importlib.rst

+        Returns the file system path to the *resource*.
+
+        If the resource does not concretely exist on the file system,
+        raise :exc:`FileNotFoundError`.


Hmm... is the same error returned when the given resource doesn't exist, or when the given resource exists but underlying storage is not a regular directory (e.g. a zip file)?

Yes. In either case, importlib.resources will know what to do with that (e.g. extract it to a temporary file in the case where it exists in the loader's view of the world, but not on the physical file system).

pitrou · 2017-12-18T10:16:10Z

Doc/library/importlib.rst

+    .. abstractmethod:: is_resource(name)
+
+        Returns ``True`` if the named *name* is considered a resource.
+        :exc:`FileNotFoundError` is raised if *name* does not exist.


And False is returned if...?

...it isn't. :) Do you think we need to clarify that?

if it was me I would actually have the method return False instead of an exception if name does not exist. That is how I do it in my own python code. At least I would think getters and setters and checks like this that returns bools should have an no throw garentee.

brettcannon added 4 commits December 8, 2017 14:42

Add code for importlib.abc.ResourceReader

d0a167c

Start of documentation for importlib.abc.ResourceReader

b4ccb2e

Nothing more than a copy of the docstrings.

Flesh out docs for importlib.abc.ResourceReader

73416f2

Tighten up what is returned by ResourceReader.contents()

4a0b916

brettcannon requested review from a team and warsaw December 15, 2017 18:42

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

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

warsaw requested changes Dec 15, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Dec 15, 2017

brettcannon added 2 commits December 15, 2017 15:31

Use 'name' instead of 'path' for contents()

8630e83

Grammar tweaks for the FLUFL

dba819b

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

Add a news entry

4169401

warsaw approved these changes Dec 15, 2017

View reviewed changes

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

Use the proper role name

ef68c33

brettcannon merged commit 4ac5150 into python:master Dec 16, 2017

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

brettcannon deleted the importlib_resources_abc branch December 16, 2017 00:29

pitrou reviewed Dec 18, 2017

View reviewed changes


		class ResourceReaderDefaultsTests(ABCTestHarness):

		SPLIT = make_abc_subclasses(ResourceReader)

Uh oh!

Conversation

brettcannon commented Dec 15, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

warsaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 15, 2017

Uh oh!

brettcannon commented Dec 15, 2017

Uh oh!

bedevere-bot commented Dec 15, 2017

Uh oh!

warsaw commented Dec 15, 2017

Uh oh!

brettcannon commented Dec 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AraHaan Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

brettcannon commented Dec 15, 2017 •

edited by bedevere-bot

Loading

AraHaan Dec 18, 2017 •

edited

Loading