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: Fix Sphinx warning in gc.rst and refactor docs clean list #103116

Merged
merged 7 commits into from
Mar 30, 2023

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Mar 29, 2023

Fix the single Sphinx warning in gc.rst, add the file to the clean list (so new warnings will fail the build), and refactor the clean list into a little Python script to keep the GitHub workflow from getting too big.

Copy link
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.

A couple comments, otherwise LGTM, thanks

Doc/tools/touch-clean-files.py Outdated Show resolved Hide resolved
Doc/tools/touch-clean-files.py Outdated Show resolved Hide resolved
hugovk and others added 2 commits March 29, 2023 22:11
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Copy link
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.

One followup comment, otherwise LGTM — thanks @hugovk !

Doc/tools/touch-clean-files.py Outdated Show resolved Hide resolved
Doc/tools/clean-files.txt Show resolved Hide resolved
Doc/tools/touch-clean-files.py Outdated Show resolved Hide resolved
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Copy link
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.

Sorry, a couple more followup fixes after @ezio-melotti 's suggested changes...

Doc/tools/touch-clean-files.py Outdated Show resolved Hide resolved
Doc/tools/touch-clean-files.py Outdated Show resolved Hide resolved
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@hugovk
Copy link
Member Author

hugovk commented Mar 30, 2023

Thanks! I've got a list of ~180 other files we can already add to clean-files.txt. Shall we do it in this PR?

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Mar 30, 2023

My inclination would be to leave that for an immediate followup PR to keep them separate in the commit history and avoid lengthening this one further, but I don't feel strongly about it and would of course defer to you and @ezio-melotti if you do.

It would be good to do as an immediate follow-up, though, as it will help avoid regressions and also merge conflicts when adding more files (which are far more likely with a short file list). Thanks!

Copy link
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.

LGTM, thanks @hugovk !

@AlexWaygood
Copy link
Member

(Doesn't need to be done in this PR.)

Maybe it would be better to keep a "dirty list" rather than a "clean list". I'd quite like to help out with getting the number of sphinx warnings down, and it would be handy to have a guaranteed-to-be-up-to-date list of all files that have yet to be worked on. Using a dirty list rather than a clean list would also mean that any new files added to the Doc/ directory would by default be tested with nitpicks enabled, whereas currently the default is for new files to be tested without nitpicks enabled.

The only downside I can see is that the dirty list would have to initially be pretty long. But I don't think that's actually a problem; it would be helpful to have an easy visualisation of the scale of the task still to be done.

@CAM-Gerlach
Copy link
Member

Yeah, that sounds like a pretty compelling to me for the aforementioned followup. If we are going to take that approach, which certainly might make sense and matches the pattern of listing files to ignore rather than check (.gitignore, etc). In that event, we would probably want to specify directory-level exclude support, as well as an include list so we could, e.g, exclude all the files in Doc/whatsnew except for the current version's, without having to manually set and update that for every branch, to minimize ongoing maintenance (since that would be permanent).

@AlexWaygood
Copy link
Member

In that event, we would probably want to specify directory-level exclude support, as well as an include list so we could, e.g, exclude all the files in Doc/whatsnew except for the current version's, without having to manually set and update that for every branch, to minimize ongoing maintenance (since that would be permanent).

You could add regex or glob support for the excludelist, but I'd personally vote against supporting an includelist and an excludelist simultaneously. That sounds like it would be overcomplicating things unnecessarily imho. Just add all files in Doc/whatsnew except 3.12.rst to the excludelist for now, and remove them one by one as we sort them out (or add a comment next to that section of the excludelist saying that it's not high priority to fix them).

We've done a very similar thing for a while for our stubtest check over at typeshed. Stubtest is a script that verifies that various things in the stub are consistent in various ways with the objects at runtime. It's a very useful tool, but it necessitates keeping a long allowlist -- some of these are inconsistencies that are still to be completed, but many have comments next to them or above them indicating that they can't be resolved for one reason or another. The system works very well for us. (Stubtest's allowlist has regex support, but not globbing support.)

https://github.com/python/typeshed/blob/main/tests/stubtest_allowlists/py3_common.txt

@hugovk
Copy link
Member Author

hugovk commented Mar 30, 2023

I'd quite like to help out with getting the number of sphinx warnings down

Great!

The only downside I can see is that the dirty list would have to initially be pretty long. But I don't think that's actually a problem; it would be helpful to have an easy visualisation of the scale of the task still to be done.

Sure. There's ~490 total RST files, ~180 clean, so ~310 dirty. Not such a big difference.

Given a dirty list, we'd have to compute the clean list, so we can touch them to rebuild them.

I made the initial clean list 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 (includes/wasm-notavail.rst and whatsnew/changelog.rst) caused a cascade effect and resulted in a number of dirty files being rebuilt too, and so failing the build. So I've removed those. We'd need a mechanism to exclude them, and potentially others.

@hugovk
Copy link
Member Author

hugovk commented Mar 30, 2023

My inclination would be to leave that for an immediate followup PR to keep them separate in the commit history and avoid lengthening this one further, but I don't feel strongly about it and would of course defer to you and @ezio-melotti if you do.

It would be good to do as an immediate follow-up, though, as it will help avoid regressions and also merge conflicts when adding more files (which are far more likely with a short file list). Thanks!

Sure, I'll merge this now and make a new PR right away!

@hugovk hugovk merged commit f192a55 into python:main Mar 30, 2023
@hugovk hugovk deleted the docs-fix-gc-warning branch March 30, 2023 18:03
@hugovk
Copy link
Member Author

hugovk commented Mar 30, 2023

➡️ #103135

miss-islington pushed a commit that referenced this pull request Mar 31, 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 `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
@hugovk
Copy link
Member Author

hugovk commented Apr 2, 2023

(Doesn't need to be done in this PR.)

Maybe it would be better to keep a "dirty list" rather than a "clean list". I'd quite like to help out with getting the number of sphinx warnings down, and it would be handy to have a guaranteed-to-be-up-to-date list of all files that have yet to be worked on. Using a dirty list rather than a clean list would also mean that any new files added to the Doc/ directory would by default be tested with nitpicks enabled, whereas currently the default is for new files to be tested without nitpicks enabled.

The only downside I can see is that the dirty list would have to initially be pretty long. But I don't think that's actually a problem; it would be helpful to have an easy visualisation of the scale of the task still to be done.

Please see GH-103191.

warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
… list (python#103116)

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
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