bpo-32248 - Implement ResourceReader and get_resource_reader() for zipimport
#5248
Conversation
warsaw
changed the title
ResourceReader and get_resource_reader() for zipimportResourceReader and get_resource_reader() for zipimport
warsaw
changed the title
ResourceReader and get_resource_reader() for zipimportResourceReader and get_resource_reader() for zipimport
warsaw
changed the title
ResourceReader and get_resource_reader() for zipimportResourceReader and get_resource_reader() for zipimport
| @@ -1,7 +1,8 @@ | |||
| Add :class:`importlib.abc.ResourceReader` as an ABC to provide a | |||
| unified API for reading resources contained within packages. Loaders | |||
| wishing to support resource reading are expected to implement the | |||
| ``get_resource_reader(fullname)`` method. | |||
| ``get_resource_reader(fullname)`` method. File-based and zipimport-based | |||
| loaders both implement these APIs. | |||
|
|
|||
| Also add :mod:`importlib.resources` as the stdlib port of the | |||
| ``importlib_resources`` PyPI package. The modules provides a high-level | |||
There was a problem hiding this comment.
This is a very long NEWS entry that looks like it should probably be in What's New :)
There was a problem hiding this comment.
I think you're right :). I'll shorten the NEWS entry and expand a bit in the What's New entry.
| self.fullname = fullname | ||
|
|
||
| def open_resource(self, resource): | ||
| path = f'{self.fullname}/{resource}' |
There was a problem hiding this comment.
Does zip importer require the forward slash? Or does it expect os.sep? (You're using pathlib below on what looks like it's probably the same style of path, but without digging further I'm not 100% sure.)
There was a problem hiding this comment.
I'm not 100% sure either, but AFAICT, the paths inside the zip all have forward slashes.
I suppose that could pose a problem with using pathlib on Windows, but given Appveyor passing, I think we can trust that it isn't a problem in practice, right?
There was a problem hiding this comment.
There is pathlib.PurePath.as_posix() to guarantee the forward slashes.
There was a problem hiding this comment.
Shouldn't os.fspath() be called on resource? And is there any worry of people accidentally passing in a subdirectory since there is no argument check?
There was a problem hiding this comment.
I don't think we need to do those things since people will be accessing these through the higher level API (I.e. importlib.resources). I think the validation done at that level means we can trust what gets passed to us. I also don't think we need to force forward slashes in the argument passed to self.zipimporter.get_data() since we're using forward slash in the f-string and zips only support forward slashes in their internal file names.
Modules/zipimport.c
Outdated
| if (function == NULL) { | ||
| return NULL; | ||
| } | ||
| PyObject *retval = PyObject_CallFunction( |
There was a problem hiding this comment.
PyObject_CallMethod can save you a GetAttrString call :)
There was a problem hiding this comment.
Also, trampolining into Python from C is my favourite feature"hack".
|
Thanks for the review @zooba ! |
| self.fullname = fullname | ||
|
|
||
| def open_resource(self, resource): | ||
| path = f'{self.fullname}/{resource}' |
There was a problem hiding this comment.
There is pathlib.PurePath.as_posix() to guarantee the forward slashes.
| self.fullname = fullname | ||
|
|
||
| def open_resource(self, resource): | ||
| path = f'{self.fullname}/{resource}' |
There was a problem hiding this comment.
Shouldn't os.fspath() be called on resource? And is there any worry of people accidentally passing in a subdirectory since there is no argument check?
|
@brettcannon Thanks for the review! |
Here's the implementation of
ResourceReaderfor zipimport. This also removes the fallback hacks for zip files, which we'll no longer need.Before you complain about the trampoline from zipimport's
get_resource_reader()into a private function inimportlib.resources, @brettcannon and I discussed this and agreed that it's an acceptable hack. See the comments in the .py files for more details.https://bugs.python.org/issue32248