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
pkgutil docs should reference glossary terms not PEP 302 #65041
Comments
The pkgutil docs still point at PEP-302 when mentioning loaders, importers, etc. They should reference the glossary terms (:term:`loader`, etc) instead. |
Will submit a patch for this. |
Submitting the patch for this issue. I observed the last message of respected Kushal but because the message was posted too earlier decided to submit patch. Thanks! |
See also bpo-26896. Apparently some of the terms you are linking are used wrongly. |
Dear Martin, Thanks! |
I’m not sure what the best solution is. I am just pointing out the problem. It seems an “importer” as mentioned in PEP-302 is more general than the current glossary definition. E.g. ImpImporter() returns an object that has a find_module() method (which partially makes it both a “meta path finder” and a “path entry finder”), but it has no load_module() method. So it fails the “importer” definition according to the current glossary. |
Note that http://bugs.python.org/issue26896 is now closed (the patches proposed in it (with some minor changes) were committed). |
Adding updated patch after merging bpo-26896. Requesting to review. Thanks! |
I still think the links are too dense. Three links to the same term in two short paragraphs is too much. Do you think it would be okay to just link the first occurrence for pkgutil.ImpImporter? Also, there is still a problem at least the definition of “finder” used by the ImpImporter documentation. In PEP-302, “a finder object has a single method: finder.find_module(fullname, path=None)”. This seems to be a pretty good description of the ImpImporter API. However, by dropping the reference to PEP-302, and linking to the glossary definition, we are suggesting that ImpImporter implements importlib.abc.MetaPathFinder and/or importlib.abc.PathEntryFinder. Both these APIs require some sort of find_spec() method, which is not provided by ImpImporter. As I see it, either Python’s “finder” glossary entry should be adjusted to accommodate PEP-302, or we should retain the PEP-302 reference and not point at the wrong definition of “finder”. |
ImpImporter was first added in changeset 37808 (https://hg.python.org/cpython/rev/ccc0b5412799) and updated a day later in changeset 37821 (https://hg.python.org/cpython/rev/3135648026c4). Thus, IMHO, we should go with your second option, Martin, and retain the PEP-302 reference in ImpImporter documentation. With regard to the accuracy of Python's 'finder' glossary entry, it was updated recently, in changeset 99434 (https://hg.python.org/cpython/rev/88cee7d16ccb), to suit PEP-302, PEP-420 and PEP-451, so ISTM there is no need to adjust it. |
I agree with the comment made by Oren. Not all instances of finder should be referenced as generic finder. Some classes and functions are are still PEP-302 specific finder. In the commit, 3987667bf98f Nick Coghlan modified the following functions to to use importlib module, thus serving the PEP-302 bind. So these can refer to glossary entry for finder. The documentation has been updated with version changed information.
The glossary entry for both finder and loader has been updated to include both PEP-302 finder, and with PEP-420 and PEP-451. With these changes already in place, I updated the patch to reflect the current state. I am proceed with the commit as this is a non-controversial change. If there are comments, please share and I will address them. |
For 3.5 version of patch, we have bring in some additional changes from bpo-26896 which properly clarified finder from importer. These changes are not required in the default branch. |
This is 3.3 version of patch attached. |
Change committed.
|
New changeset a4360b1b45a8 by Senthil Kumaran in branch 'default': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: