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

Accommodate Sphinx option by changing docstring return type of "integer" to "int". #100989

Closed
timobrembeck opened this issue Jan 12, 2023 · 16 comments
Assignees
Labels
docs Documentation in the Doc dir extension-modules C modules in the Modules dir

Comments

@timobrembeck
Copy link
Contributor

timobrembeck commented Jan 12, 2023

Bug report

When collections.deque is subclassed and the resulting class is documented with Sphinx using the option :inherited-members:, the following error appears:

WARNING: py:class reference target not found: integer -- return number of occurrences of value

I assume this is because the PyDoc_STRVAR docstring of the C-implementation is not compliant to PEP-7 as required in PyDoc_STRVAR:

PyDoc_STRVAR(count_doc,
"D.count(value) -> integer -- return number of occurrences of value");

And everything after -> is interpreted as type hint.

This can be reproduced with e.g. a python module sub_deque.py:

class SubDeque(deque):
    pass

and a documenation file docs/src/sub_deque.rst:

.. automodule:: sub_deque
   :inherited-members:

which is then built with Sphinx: sphinx-build -W docs/src docs/dist.

I'd be happy to provide a patch for this myself if you feel this issue should be fixed.

Your environment

  • CPython versions tested on: 3.7 - 3.11
  • Operating system and architecture: Arch Linux & Ubuntu

Linked PRs

@timobrembeck timobrembeck added the type-bug An unexpected behavior, bug, or error label Jan 12, 2023
timobrembeck added a commit to timobrembeck/cpython that referenced this issue Jan 12, 2023
timobrembeck added a commit to timobrembeck/cpython that referenced this issue Jan 12, 2023
@rhettinger
Copy link
Contributor

You can change "integer" to "int" if you like. The other changes look gratuitous.

Also, PEP 7 predates modern type annotations. Doc strings are allowed to put any text as the return type as long as it is interpretable by a human. It is unfortunate that Sphinx is assuming otherwise. How would it handle user defined types or types defined in type comments?

@rhettinger rhettinger changed the title Docstrings of collections.deque not compliant to PEP-7 Accommodate Sphinx option by changing docstring return type of "integer" to "int". Jan 12, 2023
@timobrembeck
Copy link
Contributor Author

Ok, thanks for your feedback!

You can change "integer" to "int" if you like. The other changes look gratuitous.

Still, changing it to D.count(value) -> int -- return number of occurrences of value would just cause Sphinx to literally search for the type int -- return number of occurrences of value which it wouldn't find either. Are you fine with including the new line after the return type? So D.count(value) -> int\n ...? Most of the other modules seem to follow that convention:

"_ncallbacks() -> int\n\

"acquire(blocking=True, timeout=-1) -> bool\n\

"get_default_domain() -> str\n\

Although to be fair, I also found quite a few other modules which use an inconsistent format, e.g.

"time() -> floating point number\n\

which I would change to time() -> float to match the correct type and allow the cross-referencing between documentations.

Also, PEP 7 predates modern type annotations.

Are there better options to annotate types in C modules? If so, I think it should be mentioned in the docs of PyDoc_STRVAR...

How would it handle user defined types or types defined in type comments?

User defined types are usually documented in the project that Sphinx is invoked in, and thus it'll find the reference to the custom type.

timobrembeck added a commit to timobrembeck/cpython that referenced this issue Feb 3, 2023
@arhadthedev arhadthedev added the extension-modules C modules in the Modules dir label Feb 3, 2023
timobrembeck added a commit to timobrembeck/cpython that referenced this issue Feb 15, 2023
@timobrembeck
Copy link
Contributor Author

@rhettinger Do you have any more thoughts on this? Anything specific you want me to change in the PR?

timobrembeck added a commit to timobrembeck/cpython that referenced this issue Feb 25, 2023
timobrembeck added a commit to timobrembeck/cpython that referenced this issue Feb 25, 2023
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
timobrembeck added a commit to timobrembeck/cpython that referenced this issue Feb 25, 2023
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
erlend-aasland pushed a commit that referenced this issue Mar 22, 2023
)

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Mar 22, 2023
…pythonGH-100990)

(cherry picked from commit c740736)

Co-authored-by: Timo Ludwig <ti.ludwig@web.de>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Mar 22, 2023
…pythonGH-100990)

(cherry picked from commit c740736)

Co-authored-by: Timo Ludwig <ti.ludwig@web.de>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington added a commit that referenced this issue Mar 22, 2023
…00990)

(cherry picked from commit c740736)

Co-authored-by: Timo Ludwig <ti.ludwig@web.de>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington added a commit that referenced this issue Mar 22, 2023
…00990)

(cherry picked from commit c740736)

Co-authored-by: Timo Ludwig <ti.ludwig@web.de>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@rhettinger
Copy link
Contributor

These edits violate the rules for writing docstrings:

Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description. The summary line may be used by automatic indexing tools; it is important that it fits on one line and is separated from the rest of the docstring by a blank line.

As a consequence, user's seeing a summary line in tool tips no longer see anything useful.

Previous tooltips:

OldRotate

New tooltips:

NewRotate

@rhettinger rhettinger reopened this Mar 22, 2023
@rhettinger
Copy link
Contributor

Also, it seems to me that the checkin title was misleading, "Improve accuracy of docstrings". In none of the cases were the docstrings inaccurate. These were almost purely stylistic edits and should not be backported.

@erlend-aasland
Copy link
Contributor

[...] As a consequence, user's seeing a summary line in tool tips no longer see anything useful.

This is correct; I see now that my review has been incomplete. @timoludwig, please create a follow-up PR that revert this regression.

Regarding backporting: we do backport doc updates/fixes, including docstrings, to bug-fix branches. This also include stylistic fixes. This reduces the risk of conflicts when other PRs are backported.

@timobrembeck
Copy link
Contributor Author

timobrembeck commented Mar 23, 2023

Sorry, I was not aware of that side effect.

Just to wrap this up: We have two conventions that are mutually exclusive: The general docstring rule says that we have to write a summary in the first line that is readable for humans and can be displayed as tooltip, and the rule for C modules says that we should write a line that can be parsed by automatic tools to retrieve the function signature.

In order to find a compromise, I see different options:

  1. Revert the changes but remove the signature-like parts from it to avoid pretending they're machine parseable (and document that the existing guideline for C module docstrings is incorrect and the general guideline applies)
  2. Revert the changes and keep the signature separated by -- and document that for C modules the preferred docstring format is signature -- summary \n\n detailed description, then ask Sphinx and other tools to conform to that convention
  3. Keep the changes and try to follow the convention signature \n\n summary \n\n detailed description when showing tool tips for functions in C modules in the interpreter

Which one of those would be your preferred solution? (Or do you have other suggestions?)

@erlend-aasland
Copy link
Contributor

IMO, docstrings should be written for humans; no matter if we're talking about extension modules or pure Python modules. The "title" line should IMO be a succinct description of the method/function; any detailed description follow two newlines below, like any well-written git commit message. See also PEP-257, which has a lot of good observations on what a good docstring is.

@timobrembeck
Copy link
Contributor Author

timobrembeck commented Mar 23, 2023

@erlend-aasland Ok, sounds reasonable. Does that mean option 1. (removing the signature from the summary line)?

Or does it depend on the complexity of the function signature, e.g. for insert(), 1. might be more readable:

  1. Return the position of *x* in the deque (at or after index *start* and before index *stop*).
  2. index(x, [start, [stop]]) -> int -- Return the position of *x* in the deque (at or after index *start* and before index *stop*).

Whereas for a simple rotate(), 2. might be preferable since it explains the occurrences of specific parameters in the description?

  1. Rotate the deque *n* steps to the right (default ``n=1``). If *n* is negative, rotates left.
  2. rotate(n) -- Rotate the deque *n* steps to the right (default ``n=1``). If *n* is negative, rotates left.

timobrembeck added a commit to timobrembeck/cpython that referenced this issue Mar 23, 2023
timobrembeck added a commit to timobrembeck/cpython that referenced this issue Mar 23, 2023
rhettinger added a commit to rhettinger/cpython that referenced this issue Mar 23, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Mar 24, 2023
…strings (pythonGH-102979)

(cherry picked from commit 7f01a11)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Mar 24, 2023
…strings (pythonGH-102979)

(cherry picked from commit 7f01a11)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
@rhettinger
Copy link
Contributor

rhettinger commented Mar 24, 2023

@timoludwig Would it solve your problem if I just removed the -> integer annotation altogether? The docstrings in listobject.c don't have such an annotation.

@rhettinger rhettinger added docs Documentation in the Doc dir and removed type-bug An unexpected behavior, bug, or error labels Mar 24, 2023
@rhettinger rhettinger self-assigned this Mar 24, 2023
miss-islington added a commit that referenced this issue Mar 24, 2023
GH-102979)

(cherry picked from commit 7f01a11)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
miss-islington added a commit that referenced this issue Mar 24, 2023
GH-102979)

(cherry picked from commit 7f01a11)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
@timobrembeck
Copy link
Contributor Author

@rhettinger Yes, Sphinx uses this regex to determine whether a docstring line is a signature, and without the ->, the following text is not matched as return type. Thus, the whole line would be correctly identified as normal text.

I could however also open a ticket on Sphinx' side to ignore everything after --. I mean this isn't documented anywhere, but as you said, the tools should follow cpython's conventions and not vice versa, and since this pattern is used in the code base for quite some time now, it could be considered a convention.

Thanks a lot for revisiting this issue!

rhettinger added a commit to rhettinger/cpython that referenced this issue Mar 24, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Mar 24, 2023
(cherry picked from commit d494091)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
@rhettinger
Copy link
Contributor

Thanks a lot for revisiting this issue!

Thanks for the report.

miss-islington added a commit that referenced this issue Mar 24, 2023
(cherry picked from commit d494091)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this issue Mar 27, 2023
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this issue Mar 27, 2023
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this issue Mar 27, 2023
warsaw pushed a commit to warsaw/cpython that referenced this issue Apr 11, 2023
warsaw pushed a commit to warsaw/cpython that referenced this issue Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir extension-modules C modules in the Modules dir
Projects
None yet
Development

No branches or pull requests

4 participants