Thanks for the review Eric.  I'm slogging through these and many other
comments, but I now have the docs integrated with trunk, and will probably
land them for better or worse in the next day or so.

I'll respond just to a few of your comments.  Whatever I omit you can consider
them "fixed"!

On Jul 28, 2012, at 06:54 AM, Eric Snow wrote:

>* (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.

I certainly struggled with this term.  I almost picked PathFinder (or "path
finder") since that's the name of the actual class used in the implementation,
but then I thought this might be too implementation specific.

You ask in [2] whether "path importer" refers specifically to the callables on
sys.path_hooks.  Can you site a reference for this?  I found one reference in
PEP 302 to "path importer" but it's hard to tell exactly what that is
referring to.  The sys module doesn't use that term.

If we agree that "path importer" is the name of the things on sys.path_hooks,
then we need a name for the thing on sys.meta_path that implements the things
on sys.path_hook. :)

>* (glossary.rst) sys path finder: having "sys" is a nice touch, making it more distinct and more explicit.

TBH, I'm not crazy about the term "sys path finder" either but I couldn't
think of anything better.

Keep the suggestions coming for both of these terms,.  I'll ruminate on it too
and leave XXX's in the docs for now.  (Maybe as I work through the rest of the
comments, something better has already been suggested.)

>* (importlib.rst) I could have sworn that find_loader() and resolve_name()
>* were public...

There's importlib.find_loader() and importlib.util.resolve_name(), but OTOH,
this is not intended to be the importlib library documentation.  So I'm happy
to leave out such details and add a reference to those docs (done).

In a subsequent comment, Nick suggests this whole chapter be called the
"Import System" instead of the "Import Machinery", but I've been thinking
"Import Protocol" might be good too.  The intent is really to describe the
hooks, methods, and attributes used by Python to accomplish import, as well as
allow Python code to extend or modify the import machinery.  The background at
the top of the chapter is really just there to set the stage (and because
afaict, nothing like that existed before :).

I'm still thinking about this.

>* (importlib.rst) SourceFileLoader.load_module(): What about when the name
>* does not match?

An ImportError gets raised?  Were you suggesting that some additional
documentation should be added for this?

>* (import_machinery.rst) import machinery, end of first paragraph: "Note that
>* importlib.import_module() is the recommended method of calling the import
>* machinery."

I rewrote the introductory paragraphs, and added a mention of
import_module(), as well as a section on importlib.  Hopefully this will
provide enough information for people to figure things out. :)

>* (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?

I've added an XXX for this.  I think the right thing to do is to update the
data model chapter, and add a link from here to there.

>* (import_machinery.rst) Meta path loaders, end of paragraph 2: "The finder
>* could also be a classmethod that returns an instance of the class."

I don't understand what you're suggesting here.

>* (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?

I don't think so.  I lifted it from somewhere (hard to remember exactly where
now ;).  PEP 302?

>* (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.

Yeah.

>* (import_machinery.rst) I'm surprised __name__ isn't required.

Indeed!  AFAICT, it was only required by the module object's default repr, but
I fixed that when I added module_repr(). :)

>* (import_machinery.rst) Meta path loaders: what should __package__ be set to
>* for a top-level module?

Great question.  I see no official recommendation in anything I've consulted,
and I think CPython is all over the map.  Some set it to None, some to the
empty string.  I added a footnote about this, and recommend the empty string,
but that's me making an executive decision. ;)

>* (import_machinery.rst) Module reprs: how does module.__qualname__ fit in?

Currently, it doesn't afaict.  The timing of the related PEPs was such that I
don't think PEP 420 ever considered __qualname__.  I'm staying silent on this
for now.

>* (import_machinery.rst) NullImporter (issue15473)?  I though Brett had a
>* plan for taking it to the gallows...

Let's cheer him on!  I have added a footnote about this based on the
discussion in that issue.

>* (import_machinery.rst) Diagrams?  Brett again.  :) He put together some
>* nice ones a few years back.

C'mon Brett, let's see 'em!

>* (simple_stmts.rst) the from list can include submodules which must be
>* imported separately, implying a step 1b

I couldn't figure out what to say differently here.

>* (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.

I think __all__ should be considered a public, official API.  Certainly it's a
defacto standard.  However, I didn't change much substantial here, so I've
pretty much left it as is.

>[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.

I've rewritten some of this, so I think the distinction is clearer.  More
clarification can perhaps come after I land this in trunk.

>[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.

Crazy!  But I've added a footnote about this.