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

gh-101100: Expand list of clean docs #103135

Merged
merged 2 commits into from
Mar 31, 2023

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Mar 30, 2023

Follow on from #103116.

Expand list of clean docs files from 3 to 181. These files have no Sphinx warnings, and their presence in this list means that any new warnings introduced will fail the build.

The list was created by subtracting the list of files with warnings from a list of all files.

I tested with all of those, but found that touching two clean files (https://github.com/python/cpython/blob/main/Doc/includes/wasm-notavail.rst and https://github.com/python/cpython/blob/main/Doc/whatsnew/changelog.rst) caused a cascade effect and resulted in a number of dirty files being rebuilt too, and failing the build. So those two have been omitted.

Automerge-Triggered-By: GH:hugovk

@CAM-Gerlach
Copy link
Member

IMO, I like @AlexWaygood 's proposal to have the list be an excludelist instead (i.e. a .nitignore 🤣 ) and then have touch all files in /Docs and possibly /Misc/NEWS that matched a certain glob (i.e. **/*.rst) and didn't match one of the paths in the excludelist? That's a little more complicated, but it ensures new files are checked by default, highlight the files that still need to be fixed, and will reduce rather than increase the length of the list over time, and we can also use comments to explicit document why certain files are excluded (e.g. the two you mention above).

I tested with all of those, but found that touching two clean files (main/Doc/includes/wasm-notavail.rst and main/Doc/whatsnew/changelog.rst) caused a cascade effect and resulted in a number of dirty files being rebuilt too, and failing the build.

Just to explain why this is, for the record—the wasm statement gets injected into numerous individual files, so when it gets touched, all those dependents need to be rebuilt too, and touching the changelog means it rebuilds it from all the source files in Misc/NEWS. And the changelog most certainly isn't clean, it just doesn't show up in the output because it is generated dynamically from numerous source files and thus evidently doesn't get checked itself (but the source files do).

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM. This is a clear improvement on the status quo, as it prevents regressions for these clean files.

IMO, I like @AlexWaygood 's proposal to have the list be an excludelist instead (i.e. a .nitignore 🤣 ) and then have touch all files in /Docs and possibly /Misc/NEWS that matched a certain glob (i.e. **/*.rst) and didn't match one of the paths in the excludelist? That's a little more complicated, but it ensures new files are checked by default, highlight the files that still need to be fixed, and will reduce rather than increase the length of the list over time, and we can also use comments to explicit document why certain files are excluded (e.g. the two you mention above).

I'm still in favour of this, but PRs are cheap. This can be all be done in a followup PR :)

@hugovk
Copy link
Member Author

hugovk commented Mar 31, 2023

Agreed, let's get this merged before new warnings appear and refactor in followup.

@miss-islington
Copy link
Contributor

Status check is done, and it's a success ✅.

@miss-islington miss-islington merged commit 20c0f19 into python:main Mar 31, 2023
@hugovk hugovk deleted the docs-expand-clean-list branch March 31, 2023 11:06
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
Follow on from python#103116.

Expand list of clean docs files from 3 to 181. These files have no Sphinx warnings, and their presence in this list means that any new warnings introduced will fail the build.

The list was created by subtracting the list of files with warnings from a list of all files.

I tested with all of those, but found that `touch`ing two clean files (https://github.com/python/cpython/blob/main/Doc/includes/wasm-notavail.rst and https://github.com/python/cpython/blob/main/Doc/whatsnew/changelog.rst) caused a cascade effect and resulted in a number of dirty files being rebuilt too, and failing the build. So those two have been omitted.

Automerge-Triggered-By: GH:hugovk
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 skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants