-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-32248: Introduce the concept of Loader.get_resource_reader() #5108
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments.
Doc/library/importlib.rst
Outdated
@@ -487,13 +492,20 @@ ABC hierarchy:: | |||
expected to be a :term:`path-like object` which represents | |||
conceptually just a file name. This means that no subdirectory | |||
paths should be included in the *resource* argument. This is | |||
because the location of the package that the loader is for acts | |||
as the "directory". Hence the metaphor for directories and file | |||
because the location of the package the reader is for acts as the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comma between "is for, acts" so it parses a little easier?
names is packages and resources, respectively. This is also why | ||
instances of this class are expected to directly correlate to | ||
a specific package (instead of potentially representing multiple | ||
packages or a module). | ||
|
||
Loaders that wish to support resource reading are expected to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
"... which returns an object implementing this ABC's interface. If the module specified by fullname is not a package, this method should return :const:None" ...?
Doc/whatsnew/3.7.rst
Outdated
@@ -288,7 +288,7 @@ importlib.resources | |||
This module provides several new APIs and one new ABC for access to, opening, | |||
and reading *resources* inside packages. Resources are roughly akin to files | |||
inside of packages, but they needn't be actual files on the physical file | |||
system. Module loaders can implement the | |||
system. Module loaders can support the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"support" used twice reads a little weird. Maybe the first one can be "provide"?
Lib/importlib/abc.py
Outdated
"""Abstract base class for loaders to provide resource reading support.""" | ||
"""Abstract base class to provide resource-reading support. | ||
|
||
Loaders who support resource reading are expected to implement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/who/that/
For loaders that wish to provide resource reading, they should now implement
get_resource_reader(fullname)
to return an object compatible withimportlib.abc.ResourceReader
.importlib.abc.ResourceLoader
has also been documented as deprecated asResourceReader
is more well-defined, full API for similar purposes.https://bugs.python.org/issue32248