gh-101100: Only show GitHub check annotations on changed doc paragraphs#108065

CAM-Gerlach

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 current HEAD with the merge base in the local main). Handy!

Issue: Fix all Sphinx reference warnings in the documentation #101100

📚 Documentation preview 📚: https://cpython-previews--108065.org.readthedocs.build/

AA-Turner

Thank you for this work! Some initial comments:

hugovk

Thank you very much for this!

A few initial comments.

CAM-Gerlach

(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 DO-NOT-MERGE just to be safe; once this is approved and otherwise ready to merge, I'll drop that commit and then queue it for automerge.)

AA-Turner

Some further comments:

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

CAM-Gerlach

Since the final changes were just to a comment and a __future__ import, I've dropped the various test commits to confirm various common and edge cases, so this should be ready to merge pending any final comments/suggestions (which hopefully aren't substantial or I'll have to temporarily add them back, heh).

AA-Turner

Thanks again for all the work on this!

A

hugovk

Thanks! Do we need to backport this?

CAM-Gerlach

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)

CAM-Gerlach

I had to manually backport #106460 due to some merge conflicts in the .nitignore, but it should be good to merge as soon as Thomas gets the chance to review it, and then we can merge and backport this (hopefully cleanly).

CAM-Gerlach

Alrighty, looks like that backport got in, so I guess we're gogo on this one at last!

miss-islington

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>

bedevere-bot

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>

CAM-Gerlach added docs Documentation in the Doc dir skip news labels Aug 16, 2023

CAM-Gerlach self-assigned this Aug 16, 2023

bedevere-bot mentioned this pull request Aug 15, 2023

Fix all Sphinx reference warnings in the documentation #101100

Open

CAM-Gerlach force-pushed the only-nitpicky-warn-on-changed-paras branch 4 times, most recently from 7bf133e to d4f66b9 Compare August 16, 2023 23:39

AA-Turner reviewed Aug 17, 2023

View reviewed changes

CAM-Gerlach force-pushed the only-nitpicky-warn-on-changed-paras branch from 37f8a2e to b6c794f Compare August 17, 2023 01:32

Only show GitHub check annotations on changed doc paragraphs

67a6dce

CAM-Gerlach force-pushed the only-nitpicky-warn-on-changed-paras branch from b6c794f to 67a6dce Compare August 17, 2023 02:37

CAM-Gerlach marked this pull request as ready for review August 17, 2023 03:20

CAM-Gerlach requested review from ezio-melotti and hugovk as code owners August 17, 2023 03:20

bedevere-bot added the awaiting core review label Aug 17, 2023

CAM-Gerlach requested a review from AA-Turner August 17, 2023 03:20

hugovk reviewed Aug 17, 2023

View reviewed changes

CAM-Gerlach requested review from abalkin and pganssle as code owners August 17, 2023 17:36

CAM-Gerlach force-pushed the only-nitpicky-warn-on-changed-paras branch from 75a2023 to 263c3c6 Compare August 17, 2023 17:47

CAM-Gerlach removed request for abalkin and pganssle August 17, 2023 18:34

CAM-Gerlach added the DO-NOT-MERGE label Aug 17, 2023

AA-Turner reviewed Aug 17, 2023

View reviewed changes

CAM-Gerlach force-pushed the only-nitpicky-warn-on-changed-paras branch 2 times, most recently from cae535a to 7a6eef9 Compare August 18, 2023 03:52

Improve check-warnings script arg parsing following Hugo's suggestions …

887ca3f

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>

CAM-Gerlach and others added 3 commits August 18, 2023 00:04

Factor filtering warnings by modified diffs into helper function

70459ca

Build docs on unmerged branch so warning lines match & avoid deep clone

d8344a9

Apply a few final review suggestions from Adam …

b006d01

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

CAM-Gerlach force-pushed the only-nitpicky-warn-on-changed-paras branch from 6c98380 to b006d01 Compare August 18, 2023 05:12

CAM-Gerlach removed the DO-NOT-MERGE label Aug 18, 2023

CAM-Gerlach requested review from AA-Turner and hugovk August 18, 2023 05:17

AA-Turner approved these changes Aug 18, 2023

View reviewed changes

hugovk approved these changes Aug 18, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 18, 2023

This was referenced Aug 18, 2023

gh-101100: Docs: Check Sphinx warnings and fail if improved #106460

Merged

[3.12] gh-101100: Docs: Check Sphinx warnings and fail if improved (GH-106460) #108116

Merged

CAM-Gerlach added the needs backport to 3.12 only security fixes label Aug 18, 2023

CAM-Gerlach merged commit eb953d6 into python:main Aug 19, 2023

bedevere-bot removed the awaiting merge label Aug 19, 2023

bedevere-bot removed the needs backport to 3.12 only security fixes label Aug 19, 2023

hugovk mentioned this pull request Sep 15, 2023

gh-109408: Run patchcheck in GitHub Actions #109459

Closed

AA-Turner mentioned this pull request Feb 3, 2025

Convert change detection to a Python script #129627

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-101100: Only show GitHub check annotations on changed doc paragraphs#108065

gh-101100: Only show GitHub check annotations on changed doc paragraphs#108065
CAM-Gerlach merged 5 commits into
python:mainfrom
CAM-Gerlach:only-nitpicky-warn-on-changed-paras

CAM-Gerlach commented Aug 16, 2023 •

edited

Loading

Uh oh!

AA-Turner left a comment

Uh oh!

hugovk left a comment

Uh oh!

CAM-Gerlach commented Aug 17, 2023

Uh oh!

AA-Turner left a comment

Uh oh!

CAM-Gerlach commented Aug 18, 2023

Uh oh!

AA-Turner left a comment

Uh oh!

hugovk left a comment

Uh oh!

CAM-Gerlach commented Aug 18, 2023

Uh oh!

CAM-Gerlach commented Aug 18, 2023

Uh oh!

CAM-Gerlach commented Aug 19, 2023

Uh oh!

miss-islington commented Aug 19, 2023

Uh oh!

bedevere-bot commented Aug 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

CAM-Gerlach commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AA-Turner left a comment

Choose a reason for hiding this comment

Uh oh!

hugovk left a comment

Choose a reason for hiding this comment

Uh oh!

CAM-Gerlach commented Aug 17, 2023

Uh oh!

AA-Turner left a comment

Choose a reason for hiding this comment

Uh oh!

CAM-Gerlach commented Aug 18, 2023

Uh oh!

AA-Turner left a comment

Choose a reason for hiding this comment

Uh oh!

hugovk left a comment

Choose a reason for hiding this comment

Uh oh!

CAM-Gerlach commented Aug 18, 2023

Uh oh!

CAM-Gerlach commented Aug 18, 2023

Uh oh!

CAM-Gerlach commented Aug 19, 2023

Uh oh!

miss-islington commented Aug 19, 2023

Uh oh!

bedevere-bot commented Aug 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CAM-Gerlach commented Aug 16, 2023 •

edited

Loading