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

Fix all Sphinx reference warnings in the documentation #101100

Open
JulienPalard opened this issue Jan 17, 2023 · 25 comments
Open

Fix all Sphinx reference warnings in the documentation #101100

JulienPalard opened this issue Jan 17, 2023 · 25 comments
Labels
docs Documentation in the Doc dir

Comments

@JulienPalard
Copy link
Member

JulienPalard commented Jan 17, 2023

Documentation

Running sphinx-build in nit-picky mode, like:

sphinx-build -n . build/html/

or:

make html SPHINXERRORHANDLING=-n

gives tons warnings. ~8k of them at the time of writing.

Most of them are innocent, like using :const: while referring to a constant defined in another file, or using :meth:`__init__` to speak about the concept of an init method.

The fall in several cases:

  • The target is not documented at all, then document it, like in 664aa94
  • There's a typo, then fix it.
  • We're mentionning something that does not exists (or no longer exists), then rewrite.
  • Innocent usage of rst markers without the intention of linking to something, then ???

I don't have statistics (yet) on the distribution for the 4 previous cases.

For the innocent usages of rst markers, there's two fixes:

  • Drop the role, :const:`IGNORECASE` becomes ``IGNORECASE``. We loose a bit of information in the rst file, I'm not fully convinced.
  • Make it point to something existing, :const:`IGNORECASE` becomes :const:`re.IGNORECASE`, or even :const:`~re.IGNORECASE` to keep the same output. It means more typing, and more links in the HTML output which are not all usefull.

My question is: should we try to fix all warnings so we can easily spot typos at build time?

I tried to see if some typo would have been avoided by the nit-picky mode by reading a few pages of WARNINGS and found just one: read1 instead of read in bz2.rst.

Linked PRs

@JulienPalard JulienPalard added the docs Documentation in the Doc dir label Jan 17, 2023
@encukou
Copy link
Member

encukou commented Jan 17, 2023

See also: https://discuss.python.org/t/broken-references-in-sphinx-docs/19463

IMO, we should fix these. Many are actual issues in the documentation.

Here's a proof-of-concept GH Action that complains about nitpicks in changed files only: encukou@e97d91f
Feel free to take it, I don't think I'll have time for this soon.

@CAM-Gerlach
Copy link
Member

IMO, we should fix these. Many are actual issues in the documentation.

Yes, definitely; I've been gradually helping on the docs I've/we've touched anyway, though it is a long term project. I've found a number of things that were undocumented, as well as a number of other real issues through that, e.g. on the sqlite3 module that Erlend and I fixed.

Most of them are innocent, like using :const: while referring to a constant defined in another file, or using :meth:__init__ to speak about the concept of an init method.

I'd argue neither is exactly innocent:

  • For the first, unless there's some particular reason not to, it usually makes sense to actually cross reference the constant, so the reader can find out more, and be explicit about it's location at least in the source (since ~ can hide that in the rendered output if not desired). This also means if the constant is renamed, moved or removed, Sphinx will issue a warning about it.
  • For the second, doing :meth:`~object.__init__` to cross reference the full description of an __init__ method will often still have value to readers, at least on first usage in a context, and doesn't take up any more space. We could potentially extend the :meth: role to automatically prepend a default name, i.e. object, to :meth: and :attr: that only have a single component, and perform the lookup with that—though the Sphinx built in roles are not the best structured to be able to extend easily without copying a fair bit of code.

For the innocent usages of rst markers, there's two fixes:

There's a third, much simpler and better fix. If the link isn't desired, every Sphinx cross-reference role supports prepending ! to prevent Sphinx from trying to resolve the reference, which avoids both the warning and the (very slight) build-time lookup cost, while preserving both all the information in the source, and the formatting in the output (since the formatting of named roles and regular literals is not the same, at least in our current theme), while being easier than both (just add one character). So, you could just do:

:const:`!IGNORECASE`

@sobolevn
Copy link
Member

sobolevn commented Jan 18, 2023

I've sent an example PR with the fix for enum module: #101122

The dev experience was plesant, because on the second run sphinx only rebuilds (and warns about) changed files:

Снимок экрана 2023-01-18 в 11 27 00

@CAM-Gerlach
Copy link
Member

As a general note of caution, especially when submitting PRs fixing these sorts of widespread and potentially nitpicky (heh) docs defects, especially when in cases like this there are a number of different possibilities to handle each warning instance, we should take care to avoid the folly of large "omnibus" PRs (as Guido likes to call them) and take care to consider each specific change we're making holistically, and ensure that we're picking the approach that best serves the reader in for each specific context, as opposed to just applying one type of fix mechanically to all instances, or even worse arbitrarily picking one or another each time without careful thought and consideration that might fix the warning but be an overall regression.

Otherwise, if we're too focused solely on the narrow objective of getting rid of the warnings by whatever means, as opposed to the broader goal of improving the overall quality of the docs, we risk both doing exactly the opposite of the latter, and consuming the limited time and churn budget of core developers and other reviewers on changes with little or even negative practical benefit.

@encukou
Copy link
Member

encukou commented Jan 19, 2023

That's the reasoning behind teaching the CI to only warn, and only on changed files.

ethanfurman pushed a commit that referenced this issue Jan 20, 2023
* gh-101100: [Enum] Fix sphinx warnings in 

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jan 20, 2023
* pythongh-101100: [Enum] Fix sphinx warnings in

(cherry picked from commit 9e025d3)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
ambv pushed a commit that referenced this issue Jan 20, 2023
…1173)

(cherry picked from commit 9e025d3)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@hugovk
Copy link
Member

hugovk commented Feb 21, 2023

To enable this, it would be really useful if Sphinx had more granular config for the warnings/errors, similar to Flake8.

For example, we could enable nitpicky only for certain directories and files, and expand as more are cleaned. And possibly in combination with an exclude option.

Similarly, we may only want to enable/disable nitpicky for certain classes of warnings/errors.

That would allow us to iteratively fix things, and keep them fixed.

cc @AA-Turner

@hugovk
Copy link
Member

hugovk commented Feb 21, 2023

PR to fix 113 warnings in the decimal module: #102125

@CAM-Gerlach
Copy link
Member

Yeah; you can silence warnings for particular names, but not in particular files, which would IMO be much more useful. Besides just incrementally fixing these issues, it would also be very useful to potentially keep permanently for What's New and Changelog pages (other than those for the current feature version in a particular branch) as those will naturally drift out of date over time. I believe I mentioned this on a Sphinx issue somewhere at some point semi-recently, but if I did I can't seem to find it now.

@timobrembeck
Copy link
Contributor

Does anybody have an opinion on the docstring-side of matters? In other words, should the docstrings inside the Python source code also comply to Sphinx's nit-picky mode? (see e.g. #100989)

@CAM-Gerlach
Copy link
Member

IMO, it's generally helpful for the docstrings to use the unambiguous, explicit and precise types if feasible, but particularly for the docstrings, avoiding warnings in -n mode seems secondary to me to ensuring they are clear, helpful and consistent for readers, per Diataxis on the overall function of reference docs, particularly since CPython itself doesn't build the docstrings. If the latter can be satisfied while serving the former purpose and being enough of a non-trivial and thoughtful improvement to not quality as mechanical churn, then it would seem to me to have net value.

In the particular case of your PR #100990 , it appears to make substantial clarity and descriptiveness improvements to the docstrings beyond just the above change (which is really secondary in benefit to the latter), so to me it appears to be a pretty clear net win.

hugovk added a commit that referenced this issue Feb 25, 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 Feb 25, 2023
…2125)

(cherry picked from commit b7c1126)

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Feb 25, 2023
…2125)

(cherry picked from commit b7c1126)

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington added a commit that referenced this issue Feb 25, 2023
(cherry picked from commit b7c1126)

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington added a commit that referenced this issue Feb 25, 2023
(cherry picked from commit b7c1126)

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
sobolevn added a commit to sobolevn/cpython that referenced this issue Feb 25, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Mar 19, 2024
…-116913)

(cherry picked from commit 9080e9e)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
hugovk added a commit that referenced this issue Mar 19, 2024
…) (#117037)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
hugovk added a commit that referenced this issue Mar 19, 2024
…) (#117038)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
vstinner pushed a commit to vstinner/cpython that referenced this issue Mar 20, 2024
adorilson pushed a commit to adorilson/cpython that referenced this issue Mar 25, 2024
* clean up fcntl module doc

* simplify

* a few changes, based on suggestion by CAM-Gerlach

* nitpick ignore for a couple other C functions mentioned in the fcntl module doc

* more changes, especially related to LOCK_* constants

* :data: back to :const:

* Apply suggestions from code review

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

---------

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
adorilson pushed a commit to adorilson/cpython that referenced this issue Mar 25, 2024
…2357)

Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
adorilson pushed a commit to adorilson/cpython that referenced this issue Mar 25, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
…5580)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
…2351)

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
* clean up fcntl module doc

* simplify

* a few changes, based on suggestion by CAM-Gerlach

* nitpick ignore for a couple other C functions mentioned in the fcntl module doc

* more changes, especially related to LOCK_* constants

* :data: back to :const:

* Apply suggestions from code review

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

---------

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
…2357)

Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
hugovk added a commit to hugovk/cpython that referenced this issue Apr 28, 2024
…thonGH-118364)

(cherry picked from commit e0ab642)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 28, 2024
…ythonGH-118353)

(cherry picked from commit 33c6cf3)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 28, 2024
…-118356)

(cherry picked from commit f5b7e39)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
hugovk added a commit that referenced this issue Apr 28, 2024
…) (#118367)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
hugovk added a commit that referenced this issue Apr 28, 2024
…H-118353) (#118366)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
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
Projects
None yet
Development

No branches or pull requests

12 participants