Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

CAM-Gerlach
Copy link
Member

@CAM-GerlachCAM-Gerlach commentedAug 16, 2023
edited
Loading

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/

hugovk reacted with rocket emoji
@CAM-GerlachCAM-Gerlach added docsDocumentation in the Doc dir skip news labelsAug 16, 2023
@CAM-GerlachCAM-Gerlach self-assigned thisAug 16, 2023
@CAM-GerlachCAM-Gerlachforce-pushed theonly-nitpicky-warn-on-changed-paras branch 4 times, most recently from7bf133e tod4f66b9CompareAugust 16, 2023 23:39
Copy link
Member

@AA-TurnerAA-Turner left a 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:

CAM-Gerlach and hugovk reacted with thumbs up emoji
@CAM-GerlachCAM-Gerlachforce-pushed theonly-nitpicky-warn-on-changed-paras branch from37f8a2e tob6c794fCompareAugust 17, 2023 01:32
@CAM-GerlachCAM-Gerlachforce-pushed theonly-nitpicky-warn-on-changed-paras branch fromb6c794f to67a6dceCompareAugust 17, 2023 02:37
@CAM-GerlachCAM-Gerlach marked this pull request as ready for reviewAugust 17, 2023 03:20
Copy link
Member

@hugovkhugovk left a 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.

@CAM-Gerlach
Copy link
MemberAuthor

(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 itDO-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.)

hugovk and AA-Turner reacted with thumbs up emoji

Copy link
Member

@AA-TurnerAA-Turner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Some further comments:

@CAM-GerlachCAM-Gerlachforce-pushed theonly-nitpicky-warn-on-changed-paras branch 2 times, most recently fromcae535a to7a6eef9CompareAugust 18, 2023 03:52
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@CAM-Gerlach
Copy link
MemberAuthor

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).

Copy link
Member

@AA-TurnerAA-Turner left a 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

CAM-Gerlach reacted with thumbs up emoji
Copy link
Member

@hugovkhugovk left a 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?

CAM-Gerlach reacted with thumbs up emoji
@CAM-Gerlach
Copy link
MemberAuthor

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 postedthis reply after the relevant comment chain had already been resolved, which you might find interesting)

@CAM-Gerlach
Copy link
MemberAuthor

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).

hugovk reacted with thumbs up emoji

@CAM-Gerlach
Copy link
MemberAuthor

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

hugovk reacted with rocket emoji

@CAM-GerlachCAM-Gerlach merged commiteb953d6 intopython:mainAug 19, 2023
@miss-islington
Copy link
Contributor

Thanks@CAM-Gerlach for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestAug 19, 2023
…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 commiteb953d6)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
Copy link

GH-108127 is a backport of this pull request to the3.12 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.12only security fixes labelAug 19, 2023
Yhg1s pushed a commit that referenced this pull requestAug 19, 2023
…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 commiteb953d6)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>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@hugovkhugovkhugovk approved these changes

@AA-TurnerAA-TurnerAA-Turner approved these changes

@ezio-melottiezio-melottiAwaiting requested review from ezio-melottiezio-melotti is a code owner

Assignees

@CAM-GerlachCAM-Gerlach

Labels
docsDocumentation in the Doc dirskip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@CAM-Gerlach@miss-islington@bedevere-bot@hugovk@AA-Turner

[8]ページ先頭

©2009-2025 Movatter.jp