Eric Snow <ericsnowcurren...@gmail.com> added the comment: Awesome addition, Barry! Bless you for slogging through this. Here are some thoughts (prepare one grain of salt for each):
* (glossary.rst) finder: should also reference PEP 420. * (glossary.rst) module: s/contain/containing/ * (glossary.rst) path importer: I like that you pointed out this specific metapath importer, but aren't path importers something else? [2] Perhaps the metapath importer doesn't need to be in the glossary. Then again I like the entry, though I'd change ":term:`finder` / :term:`loader`" to "metapath importer". Maybe just a different term would work, like "sys path subsystem". Regardless, it is certainly the big dog in the import machinery and deserves special attention. * (glossary.rst) sys path finder: having "sys" is a nice touch, making it more distinct and more explicit. * (importlib.rst) I could have sworn that find_loader() and resolve_name() were public... * (importlib.rst) module_repr() is nice. * (importlib.rst) SourceFileLoader.load_module(): What about when the name does not match? * (import_machinery.rst) import machinery: really nice intrro! * (import_machinery.rst) import machinery, end of first paragraph: "Note that importlib.import_module() is the recommended method of calling the import machinery." * (import_machinery.rst) import machinery, third paragraph: though there is the side effects of the module getting added to sys.modules, and of parent modules getting imported (if not bound). * (import_machinery.rst) package, second paragraph: "generally" implies further explanation which doesn't materialize. Perhaps s/modules generally do not contain other modules or packages/modules do not naturally contain other modules or packages/ or something like that? * (import_machinery.rst) I like that you make it clear that even packages are not strictly a FS-based construct. * (import_machinery.rst) how about a section devoted just to the attributes of modules and packages, perhaps expanding upon or supplanting the related entries in the data model reference page? * (import_machinery.rst) Namespace packages: while "provided by a separate vendor installed container" does convey the broad possibilities, it's nearly equivalent to "separate sys.path entries" in practice (and in the example). Regardless, "separate vendor installed container" could be clarified. * (import_machinery.rst) Searching, paragraph 1: don't forget importlib.import_module()! :) * (import_machinery.rst) The module cache: A gotcha snuck in under the old machinery that may or may not be worth noting. [3] * (import_machinery.rst) nice point about messing around with sys.modules. * (import_machinery.rst) I like the sound of "import protocol". * (import_machinery.rst) Meta path loaders, end of paragraph 2: "The finder could also be a classmethod that returns an instance of the class." * (import_machinery.rst) Meta path loaders: reload() is no longer a builtin function. * (import_machinery.rst) Meta path loaders: "If the load fails, the loader needs to remove any modules..." is a pretty exceptional case, since the modules is not in charge of its parent or children, nor of import statements executed for it. Is this a new requirement? * (import_machinery.rst) Meta path loaders: too bad there isn't something like "__origin__" for the case where __file__ doesn't make semantic sense, but you still want to record where the module came from. * (import_machinery.rst) I'm surprised __name__ isn't required. * (import_machinery.rst) __loader__ is finally getting the respect it deserves (after nearly 10 long years)! * (import_machinery.rst) Meta path loaders: what should __package__ be set to for a top-level module? * (import_machinery.rst) Meta path loaders: s/it should execute the module's code/the loader should execute the module's code/. * (import_machinery.rst) Module reprs: perhaps s/``loader.module_repr(module)``/``module.__loader__.module_repr(module)``/ * (import_machinery.rst) Module reprs: how does module.__qualname__ fit in? * (import_machinery.rst) module.__path__: s/are consulted/is consulted/ ? * (import_machinery.rst) The Path Importer: as noted above, this seems like a new usage of "path importer", a term which carries other meaning already. It's an important and distinct thing though, worthy of its own name. * (import_machinery.rst) sys path finders, third paragraph: maybe put a reference to the site module? * (import_machinery.rst) sys path finders, last paragraph: s/it is used to load/that's what the import machinery uses to load/. * (import_machinery.rst) NullImporter (issue15473)? I though Brett had a plan for taking it to the gallows... * (import_machinery.rst) Diagrams? Brett again. :) He put together some nice ones a few years back. * (import_machinery.rst) * (import_machinery.rst) * (simple_stmts.rst) a wonderful improvement! * (simple_stmts.rst) the from list can include submodules which must be imported separately, implying a step 1b * (simple_stmts.rst) is __all__ "considered public" in any technical way or is it just convention? The description somewhat implies that "from module import *" is asking for the module's public API. That's fine with me. Explicitly describing it as such would make that connection even more concrete. Whew. All in all, Barry, nice work on a difficult and tedious project! This is such an improvement and long overdue. Other notes: [1] A package doesn't necessarily have to correspond to a directory, does it? Meta path importers should be able to generate packages just as well as path importers. issue1644818 hints at this. [2] Doesn't "path importer" refer to the callables on sys.path_hooks that decide the path-based finder to use for a module during filesystem-based imports. In light of "sys path finder", maybe "sys path importer" is more appropriate. So "sys path finder" refers to the specialized finder used during FS-based imports. [3] A module may replace itself in sys.modules. This came up during the importlib integration when several people pointed out that they relied on this previously unspecified side-effect of the old import machinery (and importlib didn't cooperate). Django was involved, if I recall correctly. You've alluded to the situation in the footnote on import_machinery.rst. I'm pretty sure this still isn't specified, nor that it should be. And yet... Anyway, this would somewhat imply that all module attributes set by the loader should likewise be set before the module's code is executed (which _is_ already at least vaguely specified). ---------- nosy: +eric.snow _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue15295> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
