Skip to content

gh-101100: Test docs in nit-picky mode#102513

Merged
hugovk merged 13 commits intopython:mainfrom
hugovk:docs-nit-picky
Mar 24, 2023
Merged

gh-101100: Test docs in nit-picky mode#102513
hugovk merged 13 commits intopython:mainfrom
hugovk:docs-nit-picky

Conversation

@hugovk
Copy link
Copy Markdown
Member

@hugovk hugovk commented Mar 7, 2023

This builds upon @encukou's encukou#21, see also https://discuss.python.org/t/broken-references-in-sphinx-docs/19463.

This adds two new parts to the docs CI.


1. Check files modified in PR

The files are touched and the Sphinx build re-ran. Sphinx only rebuilds the modified files so is quick. But this time we run Sphinx with the -n nit-picky mode enabled to be warned about extra documentation problems.

However, we don't let it fail the build: you may have modified a file that already has lots of issues, but didn't introduce any (or many) new ones.

But we do pipe the warnings through Doc/tools/warnings-to-gh-actions.py which uses GitHub's annotations feature to highlight where exactly the warning occurs. For example, see the demo at https://github.com/encukou/cpython/pull/21/files:

Screenshot image

2. Check hardcoded list of "known good files"

We want to make sure certain files have no warnings, and remain with no warnings. To begin with, only Doc/whatsnew/3.12.rst is in this list, but we will iteratively add more as we fix them, especially "important" files.

This time, we turn the warnings into errors with -W, because we don't want these files to regress.

For example, here's 3.12.rst failing: https://github.com/hugovk/cpython/actions/runs/4347385122/jobs/7595550243

Screenshot image

This PR then also fixes 3.12.rst so the CI passes.

TODO:

  • Once merged, backport exceptions (new in 3.12), weakref, asyncio-tasks and gzip docs to 3.11/3.10.

Automerge-Triggered-By: GH:hugovk

Automerge-Triggered-By: GH:hugovk

Copy link
Copy Markdown
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

This is great; thanks @hugovk and @encukou ! I did have some comments; see below.

If adding back legitimate warnings for missing documentation results in the 3.12 What's New no longer being fully clean, you could add library/sqlite3.rst in its place, which Erlend and I have made sure should be clean (documenting what was missing, etc).

Comment thread .github/workflows/doc.yml Outdated
Comment thread .github/workflows/doc.yml Outdated
Comment thread .github/workflows/doc.yml Outdated
Comment thread .github/workflows/doc.yml Outdated
Comment thread .github/workflows/doc.yml
Comment thread Doc/whatsnew/3.12.rst
Comment thread Doc/whatsnew/3.12.rst Outdated
Comment thread Doc/whatsnew/3.12.rst Outdated
Comment thread Doc/tools/warnings-to-gh-actions.py Outdated
Comment thread Doc/tools/warnings-to-gh-actions.py Outdated
Comment thread .github/workflows/doc.yml Outdated
Comment thread .github/workflows/doc.yml Outdated
Comment thread .github/workflows/doc.yml Outdated
hugovk and others added 2 commits March 9, 2023 22:34
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@merwok
Copy link
Copy Markdown
Member

merwok commented Mar 11, 2023

Python repos don’t use force pushes

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@CAM-Gerlach
Copy link
Copy Markdown
Member

CAM-Gerlach commented Mar 12, 2023

In general, PR authors are discouraged from force-pushing once the PR is submitted to avoid making things more difficult for reviewers, but it's not an absolute blind ban—there's still some room for careful judgement, in specific situations where doing so does not conflict with, or in fact supports, the actual reasons for which we have that guideline, i.e. maximizing reviewability.

In particular, when we originally discussed this, it was agreed that it's fine to force-push before a draft PR is marked for review, as it can be a very helpful tool to organize a potentially very noisy initial commit history into an atomic, clearly-explained series of changes, which can greatly aid reviewability.

Likewise, it can potentially make sense and actually aid rather than hurt reviewability to amend a single commit immediately after pushing it to fix a small obvious typo introduced in that commit—which was the case here, with the fixup pushed less than 60 seconds after the original. There's little to no chance any reviewer would have seen, much less review/comment on the commit that quickly, thus there's minimal practical negative impact there. By contrast, it reduces unnecessary noise in the commit history and preserves a single atomic commit for reviewers to look at, without back-and-forth trivial fixups complicating things (which certainly make my life harder as a reviewer). And of course, you can still easily see both commits and compare them with one click if desired in the PR's full conversation history.

@hugovk
Copy link
Copy Markdown
Member Author

hugovk commented Mar 22, 2023

Is there anything else needed here?

Copy link
Copy Markdown
Member

@ezio-melotti ezio-melotti left a comment

Choose a reason for hiding this comment

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

Looks great!

Comment thread .github/workflows/doc.yml
Comment thread .github/workflows/doc.yml Outdated