Skip to content

gh-98586: add whatsnew entry covering limited-API outgoing vector calls, related documentation fix #99056

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Nov 6, 2022
Merged

gh-98586: add whatsnew entry covering limited-API outgoing vector calls, related documentation fix #99056

merged 7 commits into from
Nov 6, 2022

Conversation

wjakob
Copy link
Contributor

@wjakob wjakob commented Nov 3, 2022 •
edited by bedevere-bot
Loading

PR #98587 addressing issue #98586 lacked a "what's new" entry.

While making those changes, I noticed an inconsistency in how PY_VECTORCALL_ARGUMENTS_OFFSET is declared in the underlying Sphinx markup when compared to other macro constants like Py_TPFLAGS_HAVE_VECTORCALL. This PR fixes that as well, which should connect a few currently broken cross-references

@wjakob
pythongh-98586: add whatsnew entry, related documentation fix
11b4bca 
PR python#98587 addressing issue python#98586 lacked a "what's new" entry.

While making those changes, I noticed an inconsistency in how
``PY_VECTORCALL_ARGUMENTS_OFFSET`` is declared in the underlying Sphinx
markup when compared to other macro constants like
``Py_TPFLAGS_HAVE_VECTORCALL``. This PR fixes that as well, which should
connect a few currently broken cross-references
@bedevere-bot bedevere-bot mentioned this pull request Nov 3, 2022
Closed
@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir labels Nov 3, 2022
@CAM-Gerlach
Copy link
Member

FYI, you should mention in the PR title (which is also the default commit message) what you are adding a What's New entry for...

CAM-Gerlach
CAM-Gerlach reviewed Nov 3, 2022
View reviewed changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Seems like these should be changed to use equivalent roles from the appropriate domain, though I'm not sure why the Python domain has data/const but the C domain doesn't, despite C actually having "real" constants and Python only having a convention, nor what the reason was for using construct from the "wrong" domain originally...

@AA-Turner , any expert insight here?

@wjakob wjakob changed the title gh-98586: add whatsnew entry, related documentation fix gh-98586: add whatsnew entry covering limited-API outgoing vector calls, related documentation fix Nov 3, 2022
@wjakob @CAM-Gerlach
Add extra article
c493411 
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@wjakob
Copy link
Contributor Author

wjakob commented Nov 3, 2022

@CAM-Gerlach -- I updated the PR title (sorry) and merged the change with the missing article.

I am hesitant to make the domain-related changes you requested. Part of the reason is that I am following the pattern that generally seems to be used for macro constants. For example look at how Py_TPFLAGS_MANAGED_DICT is defined and referenced in the Sphinx doc (or any macro constant, as far as I can tell). If you wanted to move things over to the :c: Sphinx domain, then it would be good to do this in a concerted set of changes covering the entire documentation.

CAM-Gerlach
CAM-Gerlach approved these changes Nov 3, 2022
View reviewed changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Okay, that makes sense for now I guess. In that case, LGTM, aside from one comment

@wjakob @CAM-Gerlach
Update Doc/whatsnew/3.12.rst
862307c 
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@wjakob
Copy link
Contributor Author

wjakob commented Nov 3, 2022

aside from one comment

Incorporated, thanks.

erlend-aasland
erlend-aasland reviewed Nov 4, 2022
View reviewed changes
erlend-aasland
erlend-aasland reviewed Nov 4, 2022
View reviewed changes
@wjakob @erlend-aasland
Update Doc/whatsnew/3.12.rst
c55a974 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
erlend-aasland
erlend-aasland approved these changes Nov 4, 2022
View reviewed changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Related: we should really update the docs on best practice for writing exension modules using the Limited C API (there is already an issue for this).

@erlend-aasland erlend-aasland merged commit 57a4052 into python:main Nov 6, 2022
@wjakob wjakob deleted the whatsnew branch November 14, 2022 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CAM-Gerlach CAM-Gerlach CAM-Gerlach approved these changes

@erlend-aasland erlend-aasland erlend-aasland approved these changes

@markshannon markshannon Awaiting requested review from markshannon

@encukou encukou Awaiting requested review from encukou

Assignees
No one assigned
Labels
docs Documentation in the Doc dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wjakob @CAM-Gerlach @encukou @erlend-aasland @bedevere-bot