Skip to content
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

Jump to bottom

gh-91118: Fix docstrings that do not honor --without-doc-strings #31769

Merged
merged 12 commits into from Apr 18, 2022
Merged

gh-91118: Fix docstrings that do not honor --without-doc-strings #31769

merged 12 commits into from Apr 18, 2022
+81 −65

Conversation

arhadthedev
Copy link
Contributor

@arhadthedev arhadthedev commented Mar 8, 2022 •
edited

To support --without-doc-strings, all docstrings must be wrapped into PyDoc_STRVAR or PyDoc_STR (PEP 7). However, there are 18 occurrences in code and 10 in C API documentation that do not follow this rule. The documentation is important too because it should not teach people the wrong things.

To find the occurrences I searched for (?:^\s*.tp_doc = "|" \/\* tp_doc \*\/$) and^(?:static\s+)?const\s+char\s+[^=]+=\s*".

Fixes GH-91118.

@arhadthedev arhadthedev marked this pull request as ready for review Mar 8, 2022
JelleZijlstra
JelleZijlstra reviewed Mar 9, 2022
View changes
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

There are a bunch of others, mostly in ctypes:

% git grep '".*tp_doc\s*'
Doc/c-api/typeobj.rst:       "My objects",                   /* tp_doc */
Modules/_ctypes/_ctypes.c:    "deletes a key from a dictionary",          /* tp_doc */
Modules/_ctypes/_ctypes.c:    "metatype for the CData Objects",           /* tp_doc */
Modules/_ctypes/_ctypes.c:    "metatype for the CData Objects",           /* tp_doc */
Modules/_ctypes/_ctypes.c:    "metatype for the Pointer Objects",         /* tp_doc */
Modules/_ctypes/_ctypes.c:    "metatype for the Array Objects",           /* tp_doc */
Modules/_ctypes/_ctypes.c:    "metatype for the PyCSimpleType Objects",           /* tp_doc */
Modules/_ctypes/_ctypes.c:    "metatype for C function pointers",         /* tp_doc */
Modules/_ctypes/_ctypes.c:    "XXX to be provided",                       /* tp_doc */
Modules/_ctypes/_ctypes.c:    "Function Pointer",                         /* tp_doc */
Modules/_ctypes/_ctypes.c:    "Structure base class",                     /* tp_doc */
Modules/_ctypes/_ctypes.c:    "Union base class",                         /* tp_doc */
Modules/_ctypes/_ctypes.c:    "XXX to be provided",                       /* tp_doc */
Modules/_ctypes/_ctypes.c:    "XXX to be provided",                       /* tp_doc */
Modules/_ctypes/_ctypes.c:    "XXX to be provided",                       /* tp_doc */
Modules/_ctypes/callbacks.c:    "CThunkObject",                             /* tp_doc */
Modules/_ctypes/cfield.c:    "Structure/Union member",                   /* tp_doc */
Modules/_testcapimodule.c:    "Instantiating this exception starts infinite recursion.", /* tp_doc */

I'm a bit skeptical about how useful this is, though—does anyone actually use the configure option to strip docstrings?

Misc/NEWS.d/next/Core and Builtins/2022-03-08-21-59-57.bpo-46962.UomDfz.rst Outdated
:func:`ctypes.CopyComPointer`, :func:`ctypes.FormatError`,
:func:`ctypes.FreeLibrary`, :func:`ctypes.LoadLibrary`, and
:func:`ctypes.sizeof`, along with a bunch of methods in
:class:`~_ctypes.PyCPointerType:, :class:`~_ctypes.PyCSimpleType:, and
Copy link
Member

@JelleZijlstra JelleZijlstra Mar 9, 2022

Choose a reason for hiding this comment

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

Suggested change
:class:`~_ctypes.PyCPointerType:, :class:`~_ctypes.PyCSimpleType:, and
:class:`~_ctypes.PyCPointerType`, :class:`~_ctypes.PyCSimpleType`, and

Misc/NEWS.d/next/Core and Builtins/2022-03-08-21-59-57.bpo-46962.UomDfz.rst Outdated
:func:`ctypes.FreeLibrary`, :func:`ctypes.LoadLibrary`, and
:func:`ctypes.sizeof`, along with a bunch of methods in
:class:`~_ctypes.PyCPointerType:, :class:`~_ctypes.PyCSimpleType:, and
:class:`~_ctypes.PyCStructType:.
Copy link
Member

@JelleZijlstra JelleZijlstra Mar 9, 2022

Choose a reason for hiding this comment

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

Suggested change
:class:`~_ctypes.PyCStructType:.
:class:`~_ctypes.PyCStructType`.

Copy link
Contributor Author

@arhadthedev arhadthedev Mar 9, 2022

Choose a reason for hiding this comment

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

I'm a bit skeptical about how useful this is, though—does anyone actually use the configure option to strip docstrings?

I think that if a flag exists, it should work fully. Also it's enabled by default so strings that should not be in the binary are no longer there.

arhadthedev and others added 2 commits Mar 12, 2022
@arhadthedev arhadthedev changed the title bpo-46962: Fix docstrings that do not honor --without-doc-strings gh-91118: Fix docstrings that do not honor --without-doc-strings Apr 10, 2022
@arhadthedev arhadthedev reopened this Apr 17, 2022
@JelleZijlstra JelleZijlstra self-requested a review Apr 17, 2022
@JelleZijlstra JelleZijlstra self-assigned this Apr 17, 2022
@JelleZijlstra JelleZijlstra merged commit a573cb2 into python:main Apr 18, 2022
13 checks passed
@arhadthedev arhadthedev mentioned this pull request Apr 10, 2022
Open
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 18, 2022

Thanks @arhadthedev for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 18, 2022

Thanks @arhadthedev for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 18, 2022

Sorry, @arhadthedev and @JelleZijlstra, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker a573cb2fec664c645ab744658d7e941d72e1a398 3.9

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 18, 2022

Sorry @arhadthedev and @JelleZijlstra, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker a573cb2fec664c645ab744658d7e941d72e1a398 3.10

@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Apr 18, 2022

@arhadthedev do you want to do the backports? I feel like the docs changes are most important to backport.

@arhadthedev arhadthedev deleted the ctypes-without-doc-string branch Apr 18, 2022
arhadthedev added a commit to arhadthedev/cpython that referenced this issue Apr 18, 2022
@arhadthedev
[3.10] pythongh-91118: Fix docstrings that do not honor --without-doc…
88a96b4 
…-strings (pythonGH-31769)

Co-authored-by: Éric <merwok@netwok.org>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit a573cb2)

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 18, 2022

GH-91662 is a backport of this pull request to the 3.10 branch.

arhadthedev added a commit to arhadthedev/cpython that referenced this issue Apr 18, 2022
@arhadthedev
pythongh-91118: Fix docstrings that do not honor --without-doc-strings (
1c97e4b 
python#31769)

Co-authored-by: Éric <merwok@netwok.org>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit a573cb2)
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 18, 2022

GH-91663 is a backport of this pull request to the 3.9 branch.

@arhadthedev
Copy link
Contributor Author

@arhadthedev arhadthedev commented Apr 18, 2022

That backport to 3.9 was targeted to main instead of 3.9.

The new, correct entry is GH-91664.

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

@merwok merwok

@JelleZijlstra JelleZijlstra

Assignees

@JelleZijlstra JelleZijlstra

Labels
CLA signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix docstrings that do not honor --without-doc-strings
6 participants
@arhadthedev @miss-islington @JelleZijlstra @bedevere-bot @merwok @the-knights-who-say-ni