gh-101100: Fix dangling references in test.rst #114958

smontanaro · 2024-02-03T14:43:31Z

Further cleanup related to #114770.

One or two typos. Mostly I just suppressed the dangling references, because this module is just meant for core dev usage.

Issue: Fix all Sphinx reference warnings in the documentation #101100

📚 Documentation preview 📚: https://cpython-previews--114958.org.readthedocs.build/

Further cleanup related to python#114770.

AlexWaygood · 2024-02-04T01:12:47Z

Doc/library/test.rst

@@ -1087,12 +1087,12 @@ The :mod:`test.support.socket_helper` module provides support for socket tests.
   buildbot environment.  This method raises an exception if the
   ``sock.family`` is :const:`~socket.AF_INET` and ``sock.type`` is
   :const:`~socket.SOCK_STREAM`, and the socket has
-   :const:`~socket.SO_REUSEADDR` or :const:`~socket.SO_REUSEPORT` set on it.
+   :const:`!SO_REUSEADDR` or :const:`!SO_REUSEPORT` set on it.


The better way to fix these two warnings would be to add documentation for these constants in the socket module docs, if that documentation doesn't already exist. Then we'll also be able to reference these constants in the docs for other modules, as will users of intersphinx outside of CPython

Perhaps, but ISTR the socket module doc explicitly mentions a whole raft of constants as coming from the underlying C socket(2) implementation. That is presumably to keep from duplicating underlying canonical documentation and to avoid introducing mistakes (think platform dependencies).

I understand it would be great to have the Python library docs completely self-contained, but I don't think that's the best way to go in some cases.

Fair point, there are probably too many in the socket module for us to list them all out explicitly!

In that case, we should explicitly list them in conf.py as silenced with a comment explaining why, to avoid introducing noise here, making it easy to change the decision in the future, and ensuring we have a consistent central record of it.

Given that many of the possible constants are only given in the socket module doc as wildcards (e.g., SO_*) I don't think that will work unless you only want to list a known (small) subset.

This stuff is really at the fringe between low-level Unix stuff and the relevant Python wrappers. According to Wikipedia, the first edition of Stevens' Unix Network Programming (the network programming bible when I was a wee lad) is 768 pages long. It's not at all clear to me that there's much to be gained by documenting a few socket options. It's not really going to flatten the socket programming learning curve much. Programmers will have to delve into the C socket documentation to do anything serious anyway.

Sorry about the soapbox.

hugovk · 2024-02-04T12:15:53Z

🎉

Congratulations! You improved:

Doc/library/test.rst

Please remove from Doc/tools/.nitignore

Let's combine #114770 into this one, they need to be atomic for the CI to be green and mergeable.

Doc/library/test.rst

smontanaro · 2024-02-12T17:56:35Z

Let's combine #114770 into this one

Done. I also closed that PR.

smontanaro · 2024-02-16T20:41:45Z

Can we move this forward? Is there more people think needs to be done?

AlexWaygood · 2024-02-16T20:51:46Z

Can we move this forward? Is there more people think needs to be done?

there's a merge conflict

smontanaro · 2024-02-16T21:08:16Z

there's a merge conflict

Better now? I saw the alert in the web interface, but didn't see a conflict in my sandbox. I must have missed a step in there somewhere. I just resolved it through the web interface.

smontanaro · 2024-02-16T21:09:28Z

Once it is merged, let me try to take care of 3.12 and 3.11 (assuming these changes are backported). I have a local version of cherry_picker with some minimal color support I'd like to exercise a bit.

CAM-Gerlach

A general comment: IMO, it is much better to just leave valid warnings due to missing documentation than to take the easy way out and just hide them, as doing so defeats one of the most meaningful benefits of these warnings in the first place—identifying gaps in the docs where we haven't formally documented something that should be public. To be honest, I'd rather people not fix the trivial cases at all if that comes at the cost of ignoring the more important ones.

Doc/library/test.rst

CAM-Gerlach · 2024-02-21T04:12:42Z

Doc/library/test.rst

+   True if Python was built with the :c:macro:`!Py_DEBUG` macro defined: if
+   Python is :ref:`built in debug mode <debug-build>`


What's the reason for disabling reference resolution here, sorry? Does it not work, for some reason? A c:macro: is defined for Py_DEBUG, and the macro definition and surrounding text describes what it means for Python to be built in debug mode, so seems a useful reference here.

Also, this wording change seems a bit incongruous to my eye and the breaks don't either minimize diffs or follow the semantic structure. Therefore, I'd suggest either simplifying the structure and wording with more logical breaks:

Suggested change

True if Python was built with the :c:macro:`!Py_DEBUG` macro defined: if

Python is :ref:`built in debug mode <debug-build>`

True if Python was built with the :c:macro:`Py_DEBUG` macro defined:

i.e. it was :ref:`built in debug mode <debug-build>`

or minimizing diffs to just the essential fixes

Suggested change

True if Python was built with the :c:macro:`!Py_DEBUG` macro defined: if

Python is :ref:`built in debug mode <debug-build>`

True if Python was built with the :c:macro:`Py_DEBUG` macro

defined; that is, if

Python was :ref:`built in debug mode <debug-build>`

Probably just confusion on my part, though I think in discussing changes to test.rst we might have gone back and forth a bit. Note that the meaning and effects of debug builds are scattered in multiple locations. In my mind defining the Py_DEBUG macro is just the marker for C programmers for what you really care about, creating a debug build. That's discussed in to sections in using/configure.rst and c-api/intro.rst.

CAM-Gerlach · 2024-02-21T04:18:40Z

Doc/library/test.rst

@@ -1087,12 +1087,12 @@ The :mod:`test.support.socket_helper` module provides support for socket tests.
   buildbot environment.  This method raises an exception if the
   ``sock.family`` is :const:`~socket.AF_INET` and ``sock.type`` is
   :const:`~socket.SOCK_STREAM`, and the socket has
-   :const:`~socket.SO_REUSEADDR` or :const:`~socket.SO_REUSEPORT` set on it.
+   :const:`!SO_REUSEADDR` or :const:`!SO_REUSEPORT` set on it.


In that case, we should explicitly list them in conf.py as silenced with a comment explaining why, to avoid introducing noise here, making it easy to change the decision in the future, and ensuring we have a consistent central record of it.

Doc/library/test.rst

CAM-Gerlach · 2024-02-21T04:34:58Z

Doc/library/test.rst

@@ -475,7 +475,7 @@ The :mod:`test.support` module defines the following functions:

 .. function:: with_pymalloc()

-   Return :const:`_testcapi.WITH_PYMALLOC`.
+   Return :const:`!_testcapi.WITH_PYMALLOC`.


This is a constant in an underscored name, test module, so it's not required to document it, but given the entire description of this function is "return ", which doesn't seem terribly useful if said constant is entirely undocumented, it seems worth at least briefly touching on what that actually means here (which the warning is a canary for).

I'm guessing the reference must be suppressed in some other way, because no hyperlink shows up in the generated HTML (and no warning is emitted by Sphinx). You'll have to take this up with someone above my pay grade.

CAM-Gerlach · 2024-02-21T04:37:12Z

Doc/library/test.rst

@@ -969,7 +969,7 @@ The :mod:`test.support` module defines the following functions:

 .. function:: skip_if_broken_multiprocessing_synchronize()

-   Skip tests if the :mod:`multiprocessing.synchronize` module is missing, if
+   Skip tests if the :mod:`!multiprocessing.synchronize` module is missing, if


This is an purposefully undocumented module, I'm guessing?

I couldn't begin to guess why it's undocumented. Maybe it's meant to be semantically identical to the relevant threading module synchronization primitives?

__all__ = [ 'Lock', 'RLock', 'Semaphore', 'BoundedSemaphore', 'Condition', 'Event' ]

I see this comment preceding an import which it suggests might fail:

# Try to import the mp.synchronize module cleanly, if it fails # raise ImportError for platforms lacking a working sem_open implementation. # See issue 3770

There is also a note about this possibility in the multiprocessing module documentation.

CAM-Gerlach · 2024-02-21T04:46:29Z

Doc/library/test.rst

+   The recorder object also has a :meth:`!reset` method, which clears the
   warnings list.


This is a valid, public method of WarningsRecorder, a class publicly documented just below, that is missing its documentation. Therefore, this warning shouldn't be silenced, as again it indicates a legitimite gap in the docs. The simplest solution seems to move this line to a .. method under the WarningsRecorder class to properly document the method there, instead of overloading the already long and complex check_warnings context description with details of the object it returns.

I changed the text to :meth:``~WarningsRecorder.reset`` (how in the heck do I make the raw ReST display correctly in Markdown???) which is fine as far as that goes, but there is no documentation of either the class or the reset method. I'll leave it as-is. There will be a nitpick warning:

.../Doc/library/test.rst:1728: WARNING: py:meth reference target not found: WarningsRecorder.reset

(how in the heck do I make the raw ReST display correctly in Markdown???)

Use the fact that GitHub supports HTML tags and escape backquotes:

<code>:meth:\`~WarningsRecorder.reset\`</code>

produces :meth:`~WarningsRecorder.reset`.

CAM-Gerlach · 2024-02-21T04:46:30Z

Doc/library/test.rst

@@ -1716,7 +1716,7 @@ The :mod:`test.support.warnings_helper` module provides support for warnings tes

   In this case all warnings are caught and no errors are raised.

-   On entry to the context manager, a :class:`WarningRecorder` instance is
+   On entry to the context manager, a :class:`WarningsRecorder` instance is
   returned. The underlying warnings list from
   :func:`~warnings.catch_warnings` is available via the recorder object's
   :attr:`warnings` attribute.  As a convenience, the attributes of the object


This existing reference resolves but entirely incorrectly; it implies warnings is an "attribute" of the test.support.warnings_helper module, and furthermore actually falls back to link to the warnings stdlib module instead, which is entirely not what is intended. It should presumably be moved to where it belongs, under the class below, instead, to both avoid the misleading reference and properly document it.

This is one place where it would be real nice if Sphinx applied some simple semantics to the refs. Something which is an attribute clearly can't be a module. I've qualified the reference. As with the reset reference, it is now broken.

.../Doc/library/test.rst:1719: WARNING: py:attr reference target not found: WarningsRecorder.warnings

Both the reset and warnings items are fully qualified, so even though the links don't work, the full names are displayed.

bedevere-app · 2024-02-21T04:52:51Z

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.

And if you don't make the requested changes, you will be put in the comfy chair!

smontanaro · 2024-02-23T01:43:20Z

@CAM-Gerlach I can't see how I'm supposed to respond to your comment about the suppressed SO_REUSEADDR and SO_REUSEPORT attributes. Neither of those attributes is defined in the socket documentation (though mentioned a couple times). I'm not keen on reproducing bits of external documentation, especially as in this case where it means exactly the same thing in Python as in C.

Also note that the text near these suppressed links says basically, "Don't use these in tests!"

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>

Fix dangling references in test.rst

cbd26fc

Further cleanup related to python#114770.

bedevere-app bot added awaiting review docs Documentation in the Doc dir skip news labels Feb 3, 2024

bedevere-app bot mentioned this pull request Feb 3, 2024

Fix all Sphinx reference warnings in the documentation #101100

Open

AlexWaygood reviewed Feb 4, 2024

View reviewed changes

Doc/library/test.rst Outdated Show resolved Hide resolved

add to test.rst change pr

b9d12fd

smontanaro mentioned this pull request Feb 12, 2024

gh-101100: Update .nitignore (after accepting library/test.rst changes) #114770

Closed

Merge branch 'main' into patch-9

a6cb747

minor nit - I think this phrase read better before

2d479c6

CAM-Gerlach added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Feb 21, 2024

CAM-Gerlach requested changes Feb 21, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Feb 21, 2024

smontanaro added 4 commits February 21, 2024 18:26

restore Py_DEBUG ref

f2252d0

connect to the (undocumented) WarningsRecorder.reset method

cccb427

correct reference to WarningsRecorder class

c23ac54

links are broken, display fully qualified attributes

b3acb26

Apply suggestions from code review

38194c5

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>

hugovk removed the needs backport to 3.11 only security fixes label Apr 11, 2024

serhiy-storchaka added the needs backport to 3.13 bugs and security fixes label May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-101100: Fix dangling references in test.rst #114958

gh-101100: Fix dangling references in test.rst #114958

smontanaro commented Feb 3, 2024 •

edited by github-actions bot

Loading

AlexWaygood Feb 4, 2024 •

edited

Loading

smontanaro Feb 4, 2024

AlexWaygood Feb 4, 2024

CAM-Gerlach Feb 21, 2024

smontanaro Feb 23, 2024

smontanaro Feb 23, 2024

hugovk commented Feb 4, 2024

smontanaro commented Feb 12, 2024

smontanaro commented Feb 16, 2024

AlexWaygood commented Feb 16, 2024

smontanaro commented Feb 16, 2024

smontanaro commented Feb 16, 2024

CAM-Gerlach left a comment

CAM-Gerlach Feb 21, 2024

smontanaro Feb 22, 2024

CAM-Gerlach Feb 21, 2024

CAM-Gerlach Feb 21, 2024

smontanaro Feb 22, 2024

CAM-Gerlach Feb 21, 2024

smontanaro Feb 23, 2024

CAM-Gerlach Feb 21, 2024

smontanaro Feb 23, 2024

picnixz Jul 23, 2024

CAM-Gerlach Feb 21, 2024

smontanaro Feb 23, 2024

bedevere-app bot commented Feb 21, 2024

smontanaro commented Feb 23, 2024

		True if Python was built with the :c:macro:`!Py_DEBUG` macro defined: if
		Python is :ref:`built in debug mode <debug-build>`

-   True if Python was built with the :c:macro:`!Py_DEBUG` macro defined: if
-   Python is :ref:`built in debug mode <debug-build>`
+   True if Python was built with the :c:macro:`Py_DEBUG` macro
+   defined; that is, if
+   Python was :ref:`built in debug mode <debug-build>`

		The recorder object also has a :meth:`!reset` method, which clears the
		warnings list.

gh-101100: Fix dangling references in test.rst #114958

Are you sure you want to change the base?

gh-101100: Fix dangling references in test.rst #114958

Conversation

smontanaro commented Feb 3, 2024 • edited by github-actions bot Loading

AlexWaygood Feb 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugovk commented Feb 4, 2024

smontanaro commented Feb 12, 2024

smontanaro commented Feb 16, 2024

AlexWaygood commented Feb 16, 2024

smontanaro commented Feb 16, 2024

smontanaro commented Feb 16, 2024

CAM-Gerlach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-app bot commented Feb 21, 2024

smontanaro commented Feb 23, 2024

smontanaro commented Feb 3, 2024 •

edited by github-actions bot

Loading

AlexWaygood Feb 4, 2024 •

edited

Loading