◐ Shell
reader mode source ↗
Skip to content

bpo-38159: Clarify documentation of PyState_AddModule#16101

Merged
encukou merged 4 commits into
python:masterfrom
encukou:PyState_AddModule-doc
Nov 1, 2019
Merged

bpo-38159: Clarify documentation of PyState_AddModule#16101
encukou merged 4 commits into
python:masterfrom
encukou:PyState_AddModule-doc

Conversation

@encukou

@encukou encukou commented Sep 13, 2019 •
edited by miss-islington
Loading

Copy link
Copy Markdown
Member

This was never intented to be called manually from PyInit_*.

Also, clarify PyState_RemoveModule return value.

https://bugs.python.org/issue38159

Automerge-Triggered-By: @encukou

Automerge-Triggered-By: @encukou

@encukou
Clarify documentation of PyState_AddModule …
46cc296 
This was never intented to be called manually from PyInit_*.

Also, clarify PyState_RemoveModule return value.
@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting core review labels Sep 13, 2019
@eduardo-elizondo

eduardo-elizondo commented Sep 13, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Just to clarify "implementing an alternative import mechanism" meant someone changing __import__ right? Otherwise, could you expand a bit more on what you meant by that? :)

Btw for context, these were added since the documentation currently specifies that PyState_FindModule needs to be paired up with PyState_AddModule to work. Since the import documentation does not offer a guarantee that your module will be attached to the interpreter state (through PyState_FindModule), it seemed like the right call to have that in the init functions.

As a matter of fact, in our system we do rely on this signal to correctly work with PyState_FindModule. Our import mechanism does not do it by default.

Maybe an alternative can be to update the import documentation that it should offer this guarantee? On the other hand, it seems like that should be an implementation detail and it's best to let the module themselves call the right APIs. Thoughts?

@encukou

encukou commented Sep 13, 2019

Copy link
Copy Markdown
Member Author

Changing __import__, creating module objects dynamically, things like that. I'm not 100% sure on this expected behavior myself, unfortunately. But it's currently redundant in the common case.

encukou added a commit to eduardo-elizondo/cpython that referenced this pull request Sep 13, 2019
@encukou
Don't call PyState_AddModule …
b9a49aa 
I'm proposing a PyState_AddModule doc update to make things clearer:
python#16101
@encukou

encukou commented Sep 13, 2019

Copy link
Copy Markdown
Member Author

I asked over at at capi-sig.

@matrixise

matrixise commented Sep 13, 2019

Copy link
Copy Markdown
Member

@encukou maybe you could add "DO-NOT-MERGE"?

@encukou

encukou commented Sep 24, 2019

Copy link
Copy Markdown
Member Author

Note to reviewer: See the capi-sig discussion

@encukou

encukou commented Sep 24, 2019

Copy link
Copy Markdown
Member Author

cc @ncoghlan

eduardo-elizondo
eduardo-elizondo approved these changes Sep 24, 2019
View reviewed changes

@eduardo-elizondo eduardo-elizondo left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

LGTM with a small comment

@eduardo-elizondo eduardo-elizondo mentioned this pull request Sep 30, 2019
Merged
ncoghlan
ncoghlan reviewed Oct 8, 2019
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Added a couple of specific suggestions that I think improve precision without making it too much harder to read.

encukou and others added 2 commits October 22, 2019 13:04
@encukou @ncoghlan
Apply Nick's suggestions …
8dddb47 
Co-Authored-By: Nick Coghlan <ncoghlan@gmail.com>
@encukou
Wrap lines
904535b
@encukou

encukou commented Nov 1, 2019

Copy link
Copy Markdown
Member Author

Azure Pipelines didn't start, I'll try closing & reopening.

@encukou encukou closed this Nov 1, 2019
@encukou encukou reopened this Nov 1, 2019
@encukou encukou merged commit 9bc94ec into python:master Nov 1, 2019
@encukou encukou deleted the PyState_AddModule-doc branch November 1, 2019 15:47
@miss-islington

miss-islington commented Nov 1, 2019

Copy link
Copy Markdown
Contributor

Thanks @encukou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

@bedevere-bot

bedevere-bot commented Nov 1, 2019

Copy link
Copy Markdown

GH-17026 is a backport of this pull request to the 3.8 branch.

@bedevere-bot

bedevere-bot commented Nov 1, 2019

Copy link
Copy Markdown

GH-17027 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 1, 2019
@encukou @miss-islington
bpo-38159: Clarify documentation of PyState_AddModule (pythonGH-16101) …
ff4bfba 
This was never intented to be called manually from PyInit_*.

Also, clarify PyState_RemoveModule return value.
(cherry picked from commit 9bc94ec)

Co-authored-by: Petr Viktorin <encukou@gmail.com>
miss-islington added a commit that referenced this pull request Nov 5, 2019
@miss-islington
[3.8] bpo-38159: Clarify documentation of PyState_AddModule (GH-16101) ( …
1270d2c 
GH-17026)

This was never intented to be called manually from PyInit_*.

Also, clarify PyState_RemoveModule return value.
(cherry picked from commit 9bc94ec)


Co-authored-by: Petr Viktorin <encukou@gmail.com>


https://bugs.python.org/issue38159



Automerge-Triggered-By: @encukou
miss-islington added a commit that referenced this pull request Nov 5, 2019
@miss-islington
[3.7] bpo-38159: Clarify documentation of PyState_AddModule (GH-16101) ( …
4342af0 
GH-17027)

This was never intented to be called manually from PyInit_*.

Also, clarify PyState_RemoveModule return value.
(cherry picked from commit 9bc94ec)


Co-authored-by: Petr Viktorin <encukou@gmail.com>


https://bugs.python.org/issue38159



Automerge-Triggered-By: @encukou
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
@encukou
bpo-38159: Clarify documentation of PyState_AddModule (pythonGH-16101) …
09a8f08 
This was never intented to be called manually from PyInit_*.

Also, clarify PyState_RemoveModule return value.
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@encukou @shihai1991
bpo-38159: Clarify documentation of PyState_AddModule (pythonGH-16101) …
7243712 
This was never intented to be called manually from PyInit_*.

Also, clarify PyState_RemoveModule return value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ncoghlan ncoghlan

+1 more reviewer

@eduardo-elizondo eduardo-elizondo eduardo-elizondo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

docs Documentation in the Doc dir skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@encukou @eduardo-elizondo @matrixise @miss-islington @bedevere-bot @ncoghlan @the-knights-who-say-ni