Skip to content

Conversation

@wjakob
Copy link
Contributor

@wjakobwjakob commented Nov 3, 2022

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

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

Copy link
Member

@CAM-GerlachCAM-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?

@wjakobwjakob changed the title gh-98586: add whatsnew entry, related documentation fixgh-98586: add whatsnew entry covering limited-API outgoing vector calls, related documentation fixNov 3, 2022
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@wjakob
Copy link
ContributorAuthor

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

Copy link
Member

@CAM-GerlachCAM-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

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@wjakob
Copy link
ContributorAuthor

aside from one comment

Incorporated, thanks.

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Copy link
Contributor

@erlend-aaslanderlend-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).

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docsDocumentation in the Doc dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@wjakob@CAM-Gerlach@encukou@erlend-aasland@bedevere-bot