-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
Conversation
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
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
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 !
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
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...
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Thanks! I've got a list of ~180 other files we can already add to |
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! |
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 !
(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. |
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 ( |
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 |
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 |
Sure, I'll merge this now and make a new PR right away! |
➡️ #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
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.