Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-101100: Fix Sphinx warning in gc.rst and refactor docs clean list#103116
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
hugovk commented Mar 29, 2023 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
CAM-Gerlach left a comment
There was a problem hiding this 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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
CAM-Gerlach left a comment
There was a problem hiding this 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 !
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
CAM-Gerlach left a comment
There was a problem hiding this 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...
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
hugovk commented Mar 30, 2023
Thanks! I've got a list of ~180 other files we can already add to |
CAM-Gerlach commented Mar 30, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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! |
CAM-Gerlach left a comment
There was a problem hiding this 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 commented Mar 30, 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 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 commented Mar 30, 2023
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 ( |
AlexWaygood commented Mar 30, 2023
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 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 commented Mar 30, 2023
Great!
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 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 |
hugovk commented Mar 30, 2023
Sure, I'll merge this now and make a new PR right away! |
hugovk commented Mar 30, 2023
➡️ #103135 |
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 commented Apr 2, 2023
Please see GH-103191. |
… list (python#103116) Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
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
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.