Skip to content

bpo-40204: Update documentation formatting for Sphinx 3.0 #19397

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.

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

Closed
wants to merge 2 commits into from

Conversation

jakobandersen
Copy link

@jakobandersen jakobandersen commented Apr 6, 2020

In the recently released Sphinx v3.0 the C domain has been rewritten to improve parsing of declarations and cross-referencing capabilities. However, this means that the existing directives has gotten more strict on the type of entities they accept.
This PR updates the documentation to use the new directives and roles. Many of the changes are related to C constructs in running text where a type or expression is "referenced". These have been changed to use the new markup role c:expr which accepts arbitrary types or expressions. This is in opposition to the other roles, like c:type which is for cross-referencing a type declared with a directive.

A few issues I couldn't immediately resovle:

  • The refcount annotation in tools/extensions/c_annoations.py no longer works (though first broken in c domain: Generate node_id for objects in the right way sphinx-doc/sphinx#7269).
  • c-api/module.rst:194: the old m_reload is technically declared inside the scope of m_slots. This is now reflected in its ID/permalink.
  • c-api/call.rst: The two functions PyObject_CallFunctionObjArgs and PyObject_CallMethodObjArgs takes a variable amount of arguments, though the last one must be NULL, indicated explicitly in the declaration. This is not valid C. There could be a configuration option in Sphinx to allow this. Please open an issue for Sphinx if you find this is relevant.
  • howto/instrumentation.rst: The c:function directive is used for some markers, but without a return type. The type void was added. However, python.function.return contains return which is a C keyword, so a warning is generated. The elegant solution would perhaps be to code a custom domain for these things, as they are not really C declarations (as far as I understand).
  • c-api/unicode.rst: The function PyUnicode_Translate is documented twice.
  • c-api/init.rst: There are several instances mentioning "wchar_* string". Should this be :c:expr:`wchar_t*` string?

https://bugs.python.org/issue40204

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@jakobandersen

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

function in turn is called as follows::

status = converter(object, address);

where *object* is the Python object to be converted and *address* is the
:c:type:`void\*` argument that was passed to the :c:func:`PyArg_Parse\*` function.
:c:expr:`void*` argument that was passed to the ``PyArg_Parse*`` functions.
Copy link
Member

Choose a reason for hiding this comment

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

:c:func: not just changes the style and adds a link, but it also adds () after the function name. Please add explicit () in all sites where you changed :c:func: to :c:expr: or `` ``.

Suggested change
:c:expr:`void*` argument that was passed to the ``PyArg_Parse*`` functions.
:c:expr:`void*` argument that was passed to the ``PyArg_Parse*()`` functions.

\*`) as its argument and should return a "new" Python object, or ``NULL`` if an
error occurred.
function is called with *anything* (which should be compatible with
:c:expr:`void*`)as its argument and should return a "new" Python object, or
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:c:expr:`void*`)as its argument and should return a "new" Python object, or
:c:expr:`void*`) as its argument and should return a "new" Python object, or

error occurred.
function is called with *anything* (which should be compatible with
:c:expr:`void*`)as its argument and should return a "new" Python object, or
``NULL`` if an error occurred.
Copy link
Member

Choose a reason for hiding this comment

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

Seems the tab character is used here. Please replace all tab characters with spaces and check that lines are correctly aligned.

@@ -189,7 +189,7 @@ For convenience, some of these functions will always return a
.. c:function:: PyObject* PyErr_SetFromWindowsErr(int ierr)

This is a convenience function to raise :exc:`WindowsError`. If called with
*ierr* of :c:data:`0`, the error code returned by a call to :c:func:`GetLastError`
*ierr* of :c:expr:`0`, the error code returned by a call to :c:func:`GetLastError`
Copy link
Member

Choose a reason for hiding this comment

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

I think it is enough to use

Suggested change
*ierr* of :c:expr:`0`, the error code returned by a call to :c:func:`GetLastError`
*ierr* of ``0``, the error code returned by a call to :c:func:`GetLastError`

as for all literals.

bmwiedemann added a commit to bmwiedemann/openSUSE that referenced this pull request Jun 11, 2020
https://build.opensuse.org/request/show/812851
by user mcepl + dimstar_suse
- add requires python3-base on libpython subpackage (bsc#1167008)

- build against Sphinx 2.x until python is compatible with
  Sphinx 3.x (see gh#python/cpython#19397, bpo#40204)

- Fix build with SQLite 3.32 (bpo#40783)
  add bpo40784-Fix-sqlite3-deterministic-test.patch
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

The problem of this PR is that :c:expr: is not supported by Sphinx 2, and so this PR drops Sphinx 2 support. Since Sphinx 3 is not widely adopted yet, I proposed a different approach which keeps Sphinx 2 support, but adds Sphinx 3 support, see: https://bugs.python.org/issue40204 Sphinx 2+3 support can be added to stable branches, it's not a backward incompatible change. But my changes require Sphinx 3.2 which adds a new option to opt-in for Sphinx 2 syntax for the C domain.

This PR contains a few relevant changes which are compatible with Sphinx 2. If you want, please try to extract them and write a new PR.

@bedevere-bot
Copy link

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.

@vstinner vstinner closed this Aug 17, 2020
@vstinner
Copy link
Member

Thanks for your contribution! But your PR couldn't be merged. If you want, please try to extract your changes which are relevant and compatible with Sphinx 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes docs Documentation in the Doc dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants