-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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: Only show GitHub check annotations on changed doc paragraphs #108065
gh-101100: Only show GitHub check annotations on changed doc paragraphs #108065
Conversation
7bf133e
to
d4f66b9
Compare
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.
Thank you for this work! Some initial comments:
37f8a2e
to
b6c794f
Compare
b6c794f
to
67a6dce
Compare
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.
Thank you very much for this!
A few initial comments.
75a2023
to
263c3c6
Compare
(Just to note, I made another commit to continue to test that the annotations appear (and don't appear) as expected on added files, and on added, fixed and modified warnings in modified files. It deliberately fails the new-warnings check, and I also marked it |
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.
Some further comments:
cae535a
to
7a6eef9
Compare
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
6c98380
to
b006d01
Compare
Since the final changes were just to a comment and a |
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.
Thanks again for all the work on this!
A
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.
Thanks! Do we need to backport this?
I was actually just about to comment on this now as I remembered that I'd forgotten to do so yesterday. The same problems this fixes (warning spam, as well as line numbers being off, etc) will be present in the Files Changed views on the 3.12 branch, where the script is present (its not on 3.11). We will need to backport PR #106460 first though as it makes comprehensive changes to these same files; its backport was deferred to after the reusable docs changes were merged, which they now are, so it seems we should be able to just go ahead with that backport first, and then this should backport cleanly. (In case you didn't see it, I posted this reply after the relevant comment chain had already been resolved, which you might find interesting) |
I had to manually backport #106460 due to some merge conflicts in the |
Alrighty, looks like that backport got in, so I guess we're gogo on this one at last! |
Thanks @CAM-Gerlach for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
…ragraphs (pythonGH-108065) * Only show GitHub check annotations on changed doc paragraphs * Improve check-warnings script arg parsing following Hugo's suggestions * Factor filtering warnings by modified diffs into helper function * Build docs on unmerged branch so warning lines match & avoid deep clone --------- (cherry picked from commit eb953d6) Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM> Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
GH-108127 is a backport of this pull request to the 3.12 branch. |
…aragraphs (GH-108065) (#108127) gh-101100: Only show GitHub check annotations on changed doc paragraphs (GH-108065) * Only show GitHub check annotations on changed doc paragraphs * Improve check-warnings script arg parsing following Hugo's suggestions * Factor filtering warnings by modified diffs into helper function * Build docs on unmerged branch so warning lines match & avoid deep clone --------- (cherry picked from commit eb953d6) Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM> Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
PR #102513 implemented support for displaying Sphinx warnings on changed files inline in the GitHub UI, which can be a useful aid to authors that they've fixed any defects on the lines they've touched and not introduced new ones.
However, annotations are shown on an entire file, regardless of whether the line the warning was issued on was within or even near those actually modified by the PR. While this has helped motivate large-scale cleanup of many of these warnings, as I commented when this was first deployed it has naturally led to a serious amount of noise, annoyance and confusion for PR authors seeing warnings on lines they had nothing to do with, which in turn leads to the understandable temptation for authors to ignore valid warnings or just silencing them, hiding rather than fixing the underlying defect.
The solution is, naturally, to only show warnings on modified lines, which is somewhat complicated by the fact that warning lines are only necessarily accurate at the paragraph level, requiring some additional processing. After much delay and burnout, I've finally gotten around to implementing it myself with this PR.
As a bonus, the script now works equally as well locally as remotely; just pass the commits/branches/tags to compare with
--check-and-annotate-changes <base> <branch>
, or use the default (comparing the changes on the currentHEAD
with the merge base in the localmain
). Handy!📚 Documentation preview 📚: https://cpython-previews--108065.org.readthedocs.build/