bpo-37800: Clean up importlib documentation for some module attributes by geryogam · Pull Request #10016 · python/cpython · GitHub

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__).

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.

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

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

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

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.

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

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

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.

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

@warsaw are you available to review again?

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

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.

Co-authored-by: Brett Cannon <brett@python.org>

Two minor tweaks, but then I believe this can be merged once the open conversations are resolved and the trailing whitespace is dealt with.

Thanks @brettcannon, I have made the requested changes; please review again.

About the three remaining open conversations with @ericsnowcurrently, I have addressed them but got no feedback since. Should I mark them resolved myself?

Thanks for making the requested changes!

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

@maggyero if you believe you are resolved you can mark them as resolved yourself.

@maggyero if you believe you are resolved you can mark them as resolved yourself.

Alright, but since he was heavily involved in the review of this PR and he is the author of PEP 451, I prefer to ask him a last time before we merge this. @ericsnowcurrently, if you are not too busy, do you agree with my updates in the three remaining open conversations? If you are still interested in seeing this PR merged, please let me know. Otherwise I am fine with closing it.

Sorry I didn't get back to this sooner! You've been so patient. Thank you.

There are just a few things that hadn't been addressed since my last review. I've add a few other minor suggestions.

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.

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 <ericsnowcurrently@gmail.com>

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.

Thanks for making the requested changes!

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

@ericsnowcurrently can you have a look?

Yeah, sorry, today or tomorrow.

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!

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

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

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

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

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 ericsnowcurrently and warsaw 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 brettcannon and warsaw April 9, 2019 22:54

bedevere-bot added the awaiting changes label Jul 23, 2021

geryogam and others added 3 commits July 23, 2021 19:40

Update Doc/library/importlib.rst

ce8d9ac

Co-authored-by: Brett Cannon <brett@python.org>

Update Doc/library/importlib.rst

0cc0b11

Co-authored-by: Brett Cannon <brett@python.org>

Update importlib.rst

69d187d

bedevere-bot removed the awaiting changes label Jul 23, 2021

Update importlib.rst

3461332

ericsnowcurrently requested changes Jul 29, 2021

View reviewed changes

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

Apply suggestions from code review

19b0542

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

Update importlib.rst

5094cc5

Update importlib.rst

7bd4866

Update importlib.rst

fc40bb0

Update importlib.rst

60dc201

ericsnowcurrently approved these changes Sep 4, 2021

View reviewed changes

Comment thread Doc/library/importlib.rst Outdated

Comment thread Doc/library/importlib.rst Outdated

Comment thread Doc/library/importlib.rst

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

Update Doc/library/importlib.rst

fcea8dd

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

Update Doc/library/importlib.rst

2bb2c14

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

JulienPalard mentioned this pull request Apr 27, 2022

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

Closed

bpo-37800: Clean up importlib documentation for some module attributes by geryogam · Pull Request #10016 · python/cpython · GitHub

Repository navigation

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

bpo-37800: Clean up importlib documentation for some module attributes#10016miss-islington merged 48 commits intopython:mainpython/cpython:mainfrom geryogam:patch-1geryogam/cpython:patch-1Copy head branch name to clipboard

Conversation

geryogam commented Oct 20, 2018 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlnub123 commented Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geryogam commented Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlnub123 commented Oct 21, 2018

Uh oh!

geryogam commented Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlnub123 commented Oct 21, 2018

Uh oh!

geryogam commented Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

warsaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Nov 18, 2018

Uh oh!

geryogam commented Nov 26, 2018

Uh oh!

brettcannon commented Apr 11, 2019

Uh oh!

brettcannon commented Apr 26, 2019

Uh oh!

bedevere-bot commented Jul 23, 2021

Uh oh!

geryogam commented Jul 23, 2021

Uh oh!

bedevere-bot commented Jul 23, 2021

Uh oh!

brettcannon commented Jul 26, 2021

Uh oh!

geryogam commented Jul 27, 2021

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Jul 29, 2021

Uh oh!

ericsnowcurrently commented Jul 29, 2021

Uh oh!

geryogam commented Jul 29, 2021

Uh oh!

bedevere-bot commented Jul 29, 2021

Uh oh!

brettcannon commented Aug 27, 2021

Uh oh!

ericsnowcurrently commented Sep 2, 2021

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bpo-37800: Clean up importlib documentation for some module attributes#10016
miss-islington merged 48 commits into
python:mainfrom
geryogam:patch-1

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