GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-26515: Update C API docs to use PyModuleDef_Init() by berkerpeksag · Pull Request #8682 · python/cpython

Conversation

@berkerpeksag

@berkerpeksag berkerpeksag commented Aug 6, 2018 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue26515

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@berkerpeksag
bpo-26515: Update C API docs to use PyModuleDef_Init()
351e087
@berkerpeksag berkerpeksag added needs backport to 3.6 skip news labels Aug 6, 2018
@berkerpeksag
berkerpeksag requested review from brettcannon and ncoghlan August 6, 2018 02:57
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 6, 2018
@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting merge labels Aug 6, 2018
@brettcannon
brettcannon requested a review from encukou August 10, 2018 19:01
ncoghlan
ncoghlan reviewed Aug 22, 2018
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This looks like a nice improvement to me. There's a forward reference that I don't think is quite right any more, and one small syntactic tweak needed to fix the build.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/extending/extending.rst
}

then we need to specify the :c:func:`spam_exec` function for the
c:var:`Py_mod_exec` slot::

@ncoghlan ncoghlan Aug 22, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Missing the leading : here (that appears to be what the CI error is complaining about)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/extending/extending.rst

We discuss the use of ``PyMODINIT_FUNC`` as a function return type later in this
sample.
We discuss the use of :c:macro:`PyMODINIT_FUNC` as a function return type later

@ncoghlan ncoghlan Aug 22, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

With the modified example, this macro hasn't actually been referenced yet. Instead, the forward reference needs to explain that this list of slots will then be added to a PyModuleDef struct defining the module as a whole, which is described later.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
encukou
encukou requested changes Sep 12, 2018
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry for getting to this so late -- I missed the review request notification.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/extending/extending.rst
@@ -204,24 +204,34 @@ usually declare a static object variable at the beginning of your file::

static PyObject *SpamError;

@encukou encukou Sep 12, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is a problem if the module is loaded more than once (for example, in multiple subinterpreters).
In that case, spam_exec would be executed multiple times, each time calling PyErr_NewException and setting SpamError to the newly created error object.

This pointer needs to be part of per-module state.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting merge label Sep 12, 2018
@bedevere-bot

bedevere-bot commented Sep 12, 2018

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be put in the comfy chair!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Sep 12, 2018
@vstinner vstinner removed the needs backport to 3.6 label Jan 10, 2019
@vstinner

vstinner commented Jan 10, 2019

Copy link
Copy Markdown
Member

I removed the " needs backport to 3.6" label, the 3.6 branch no longer accept bugfixes (only security fixes are accepted): https://devguide.python.org/#status-of-python-branches
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@csabella

csabella commented Feb 19, 2019

Copy link
Copy Markdown
Contributor

@berkerpeksag It looks like this one was close to being merged. When you have a chance, please review the changes requested by @ncoghlan and @encukou. Thanks!
brettcannon reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon
brettcannon removed their request for review March 1, 2019 20:43
@JulienPalard

JulienPalard commented Sep 10, 2019

Copy link
Copy Markdown
Member

@berkerpeksag, ping
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag

berkerpeksag commented Sep 11, 2019

Copy link
Copy Markdown
Member Author

I'll update my branch this weekend. Sorry for my late response!
JulienPalard reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@csabella csabella assigned berkerpeksag Nov 20, 2019
@encukou

encukou commented Dec 29, 2020

Copy link
Copy Markdown
Member

More detailed discussion/insructions is in PEP 630.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag

berkerpeksag commented Jan 6, 2021

Copy link
Copy Markdown
Member Author

@encukou so is this still needed or is PEP 630 the main reference point now?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou

encukou commented Jan 12, 2021

Copy link
Copy Markdown
Member

Because of PEP 630's open issues, making PyModuleDef_Init the default way to make classes would be leading some users into a trap. I am still not comfortable making it a default in the docs, and I'd rather have users read PEP 630 and understand the benefits and tradeoffs.
Until the open issues are solved, of course.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag

berkerpeksag commented Jan 12, 2021

Copy link
Copy Markdown
Member Author

Thank you. Closing this for now then.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag berkerpeksag closed this Jan 12, 2021
@brettcannon brettcannon mentioned this pull request Apr 10, 2022
Update extending/embedding docs to new way to build modules in C #70702
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ncoghlan ncoghlan ncoghlan left review comments
@encukou encukou encukou requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@berkerpeksag berkerpeksag

Labels

awaiting changes docs Documentation in the Doc dir skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@berkerpeksag @bedevere-bot @vstinner @csabella @JulienPalard @encukou @ncoghlan @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.