bpo-37800: Clean up importlib documentation for some module attributes #10016

geryogam · 2018-10-20T23:02:30Z

https://bugs.python.org/issue37800

Automerge-Triggered-By: GH:brettcannon

* The ``loader`` attribute is not `None` for namespace packages (see https://stackoverflow.com/questions/52869541/namespace-package-loader-attribute-not-set-to-none). * The ``submodule_search_locations `` attribute is not `None` for non-package modules, it is not set (see https://docs.python.org/3/reference/import.html#__path__). * The ``cached `` attribute is not sometimes `None`, it is sometimes not set (see https://docs.python.org/3/reference/import.html#__cached__). * The ``parent `` attribute is not `None` but the empty string for non-package top-level modules (see https://docs.python.org/3/reference/import.html#__package__).

orlnub123 · 2018-10-21T09:58:48Z

There seems to be some misconceptions here, a ModuleSpec object is different from the attributes set on modules. Those attributes do get set (or not set) based on the ModuleSpec object but the docs aren't referring to them.

I think the confusion aroused from the format in which the docs present this information. The module's attributes don't exactly reflect the ModuleSpecs' from which they originate from as you've said, yet the docs act like they do. If there is a change to be had, I'd suggest clarifying that they aren't always equal or, if the behavior of __loader__ for namespace packages changes, that they aren't always defined.

geryogam · 2018-10-21T10:55:51Z

@orlnub123 I disagree here. The values of the attributes of a module and the attributes of its __spec__ attribute are equivalent, it is just that there are not synchronized, as the documentation already states clearly:

Note however that while the values are usually equivalent, they can differ since there is no synchronization between the two objects. Thus it is possible to update the module’s __path__ at runtime, and this will not be automatically reflected in __spec__.submodule_search_locations.

Try to import a namespace package and you will see that both package.__spec__.loader and package.__loader__ attributes are not None, contrary to what the documentation states.

And you can verify my three other claims as well.

orlnub123 · 2018-10-21T12:32:19Z

My bad, I didn't know the spec's loader got mutated which is a bit intriguing to be honest. I do agree that they are equivalent after learning about that, guess I had some misconceptions of my own.

I still do think there is some confusion. Where the docs state that they can be set to None, it's referring to the attributes on the spec. For example if the spec sets submodule_search_locations to None, the corresponding module's __path__ attribute doesn't get defined. That doesn't mean you can return a spec which doesn't have a submodule_search_locations attribute defined, that would lead to an error.

One thing I did notice that is indeed wrong is that the ModuleSpec's parent attribute never returns None. According to PEP 366 setting it to None is fine, as the import system corrects it later on. This makes me question whether the docs are there to help you create your own arbitrary spec object, or to specifically document the ModuleSpec.

…d` attributes

geryogam · 2018-10-21T16:12:35Z

@orlnub123 My bad too, my claims for the submodule_search_locations and the cached attributes were wrong. I only checked the representation of the module.__spec__ attribute, which does not display its None attributes, so I thought there were not set, but calling explicitly module.__spec__. submodule_search_locations and module.__spec__.cached does yield None (not an AttributeError). I have updated the pull request and its description accordingly.
My other claims for the loader and the parent attributes still hold true.

orlnub123 · 2018-10-21T18:09:40Z

The loader attribute is a bit of an edge case. You would initially set it to None when creating the spec for namespace packages as talked about in PEP 451, but it would then get mutated to a non-none value after module creation. I don't think removing any mention about this would be a good idea.

geryogam · 2018-10-21T18:39:08Z

@orlnub123 Thank you for the PEP reference. I have just emailed the author Eric Snow so that he could give us his thought.

warsaw

Thanks for helping to improve the documentation. I like your changes, although I have a few grammatical suggestions.

Doc/library/importlib.rst

bedevere-bot · 2018-11-18T21:45:40Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

geryogam · 2018-11-26T21:58:03Z

@warsaw Thank you for the review. I have updated the pull request.

brettcannon · 2019-04-11T21:15:50Z

@warsaw are you available to review again?

brettcannon · 2019-04-26T23:01:19Z

While we wait to hear back from @warsaw , one comment I have is there seems to be a loss of detail on the ModuleSpec docs but an increase on the load_module() docs which is actually the reverse of how I would expect it to go (especially since load_module() is deprecated).

bedevere-bot · 2021-07-29T18:37:16Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ericsnowcurrently · 2021-07-29T18:40:17Z

If you are still interested in seeing this PR merged, please let me know. Otherwise I am fine with closing it.

Yeah, this is worth following through. Sorry it's dragged on so long. I'm going to prioritize getting this wrapped up ASAP.

Co-authored-by: Eric Snow <[email protected]>

geryogam · 2021-07-29T23:07:04Z

I have made the requested changes; please review again.

If you are still interested in seeing this PR merged, please let me know. Otherwise I am fine with closing it.

Yeah, this is worth following through. Sorry it's dragged on so long. I'm going to prioritize getting this wrapped up ASAP.

Thanks for looking at this @ericsnowcurrently! I have applied all your suggestions.

bedevere-bot · 2021-07-29T23:07:09Z

Thanks for making the requested changes!

@ericsnowcurrently, @brettcannon, @warsaw, @JulienPalard: please review the changes made to this pull request.

brettcannon · 2021-08-27T19:54:31Z

@ericsnowcurrently can you have a look?

ericsnowcurrently · 2021-09-02T20:27:04Z

Yeah, sorry, today or tomorrow.

ericsnowcurrently

This looks good. Thanks so much for your patience, thoughtfulness, and diligence!!!

I've left a few comments, one of which is a minor nit, one is a potentially incorrect (or confusing) phrase, and the last is just a reminder (with nothing to change). I'm approving the PR under the assumption that you will make any necessary corrections before this gets merged (in case I'm not the one to merge it).

Thanks again!

Doc/library/importlib.rst

Co-authored-by: Eric Snow <[email protected]>

miss-islington · 2021-11-16T19:59:44Z

@maggyero: Status check is done, and it's a success ✅ .

geryogam · 2021-11-16T20:10:19Z

@brettcannon Thanks for helping us complete this PR! @ericsnowcurrently Thanks for your multiple careful reviews! And thanks to the other reviewers.

ericsnowcurrently · 2021-11-16T21:42:53Z

Thanks for your patience, @maggyero! Congrats on getting this merged.

pythonGH-10016) Automerge-Triggered-By: GH:brettcannon

the-knights-who-say-ni added the CLA signed label Oct 20, 2018

bedevere-bot added docs Documentation in the Doc dir awaiting review labels Oct 20, 2018

Update the description of the submodule_search_locations and `cache…

7113766

…d` attributes

geryogam changed the title ~~Correct the description of ModuleSpec attributes~~ Correct the description of the ModuleSpec attributes Oct 21, 2018

Correct a typo

11ed136

geryogam changed the title ~~Correct the description of the ModuleSpec attributes~~ bpo-35181: Correct the description of the ModuleSpec attributes Nov 7, 2018

geryogam changed the title ~~bpo-35181: Correct the description of the ModuleSpec attributes~~ bpo-35181: Correct the description of the _bootstrap.ModuleSpec attributes Nov 7, 2018

brettcannon requested review from warsaw and ericsnowcurrently November 7, 2018 19:33

geryogam changed the title ~~bpo-35181: Correct the description of the _bootstrap.ModuleSpec attributes~~ bpo-35181: Correct the description of ModuleSpec attributes Nov 7, 2018

warsaw requested changes Nov 18, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Nov 18, 2018

Fix and improve wording

9a8d8e6

geryogam added 2 commits November 26, 2018 23:28

Document the read-only property for spec.parent

859ff69

Add information on the loader_state and has_location attributes

1f63f5b

csabella requested review from warsaw and brettcannon April 9, 2019 22:54

geryogam and others added 5 commits July 29, 2021 23:57

Apply suggestions from code review

19b0542

Co-authored-by: Eric Snow <[email protected]>

Update importlib.rst

5094cc5

Update importlib.rst

7bd4866

Update importlib.rst

fc40bb0

Update importlib.rst

60dc201

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 29, 2021

bedevere-bot requested a review from ericsnowcurrently July 29, 2021 23:07

ericsnowcurrently approved these changes Sep 4, 2021

View reviewed changes

Doc/library/importlib.rst Outdated Show resolved Hide resolved

Doc/library/importlib.rst Outdated Show resolved Hide resolved

Doc/library/importlib.rst Show resolved Hide resolved

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 4, 2021

brettcannon and others added 2 commits November 16, 2021 11:42

Update Doc/library/importlib.rst

fcea8dd

Co-authored-by: Eric Snow <[email protected]>

Update Doc/library/importlib.rst

2bb2c14

Co-authored-by: Eric Snow <[email protected]>

brettcannon added 🤖 automerge and removed 🤖 automerge labels Nov 16, 2021

miss-islington merged commit d7e2100 into python:main Nov 16, 2021

bedevere-bot removed the awaiting merge label Nov 16, 2021

geryogam deleted the patch-1 branch November 16, 2021 20:02

remykarem pushed a commit to remykarem/cpython that referenced this pull request Dec 7, 2021

bpo-37800: Clean up importlib documentation for some module attributes (

50fea6c

pythonGH-10016) Automerge-Triggered-By: GH:brettcannon

JulienPalard mentioned this pull request Apr 27, 2022

Doc: Namespace Packages: Inconsistent documentation of __loader__ being None #79362

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-37800: Clean up importlib documentation for some module attributes #10016

bpo-37800: Clean up importlib documentation for some module attributes #10016

geryogam commented Oct 20, 2018 •

edited by miss-islington

Loading

orlnub123 commented Oct 21, 2018 •

edited

Loading

geryogam commented Oct 21, 2018 •

edited

Loading

orlnub123 commented Oct 21, 2018

geryogam commented Oct 21, 2018 •

edited

Loading

orlnub123 commented Oct 21, 2018

geryogam commented Oct 21, 2018 •

edited

Loading

warsaw left a comment

bedevere-bot commented Nov 18, 2018

geryogam commented Nov 26, 2018

brettcannon commented Apr 11, 2019

brettcannon commented Apr 26, 2019

bedevere-bot commented Jul 29, 2021

ericsnowcurrently commented Jul 29, 2021

geryogam commented Jul 29, 2021

bedevere-bot commented Jul 29, 2021

brettcannon commented Aug 27, 2021

ericsnowcurrently commented Sep 2, 2021

ericsnowcurrently left a comment

miss-islington commented Nov 16, 2021

geryogam commented Nov 16, 2021

ericsnowcurrently commented Nov 16, 2021

bpo-37800: Clean up importlib documentation for some module attributes #10016

bpo-37800: Clean up importlib documentation for some module attributes #10016

Conversation

geryogam commented Oct 20, 2018 • edited by miss-islington Loading

orlnub123 commented Oct 21, 2018 • edited Loading

geryogam commented Oct 21, 2018 • edited Loading

orlnub123 commented Oct 21, 2018

geryogam commented Oct 21, 2018 • edited Loading

orlnub123 commented Oct 21, 2018

geryogam commented Oct 21, 2018 • edited Loading

warsaw left a comment

Choose a reason for hiding this comment

bedevere-bot commented Nov 18, 2018

geryogam commented Nov 26, 2018

brettcannon commented Apr 11, 2019

brettcannon commented Apr 26, 2019

bedevere-bot commented Jul 29, 2021

ericsnowcurrently commented Jul 29, 2021

geryogam commented Jul 29, 2021

bedevere-bot commented Jul 29, 2021

brettcannon commented Aug 27, 2021

ericsnowcurrently commented Sep 2, 2021

ericsnowcurrently left a comment

Choose a reason for hiding this comment

miss-islington commented Nov 16, 2021

geryogam commented Nov 16, 2021

ericsnowcurrently commented Nov 16, 2021

geryogam commented Oct 20, 2018 •

edited by miss-islington

Loading

orlnub123 commented Oct 21, 2018 •

edited

Loading

geryogam commented Oct 21, 2018 •

edited

Loading

geryogam commented Oct 21, 2018 •

edited

Loading

geryogam commented Oct 21, 2018 •

edited

Loading