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: Test docs in nit-picky mode#102513

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

Merged
hugovk merged 13 commits intopython:mainfromhugovk:docs-nit-picky
Mar 24, 2023
Merged

Conversation

hugovk
Copy link
Member

@hugovkhugovk commentedMar 7, 2023
edited
Loading

This builds upon@encukou'sencukou#21, see alsohttps://discuss.python.org/t/broken-references-in-sphinx-docs/19463.

This adds two new parts to the docs CI.


1. Check files modified in PR

The files aretouched and the Sphinx build re-ran. Sphinx only rebuilds the modified files so is quick. But this time we run Sphinx with the-n nit-picky mode enabled to be warned about extra documentation problems.

However, we don't let it fail the build: you may have modified a file that already has lots of issues, but didn't introduce any (or many) new ones.

But we do pipe the warnings throughDoc/tools/warnings-to-gh-actions.py which uses GitHub's annotations feature to highlight where exactly the warning occurs. For example, see the demo athttps://github.com/encukou/cpython/pull/21/files:

Screenshotimage

2. Check hardcoded list of "known good files"

We want to make sure certain files have no warnings, and remain with no warnings. To begin with, onlyDoc/whatsnew/3.12.rst is in this list, but we will iteratively add more as we fix them, especially "important" files.

This time, we turn the warnings into errors with-W, because we don't want these files to regress.

For example, here's3.12.rst failing:https://github.com/hugovk/cpython/actions/runs/4347385122/jobs/7595550243

Screenshotimage

This PR then also fixes3.12.rst so the CI passes.

TODO:

  • Once merged, backportexceptions (new in 3.12), weakref, asyncio-tasks and gzip docs to 3.11/3.10.

Automerge-Triggered-By: GH:hugovk

Automerge-Triggered-By: GH:hugovk

m-aciek reacted with thumbs up emoji
Copy link
Member

@CAM-GerlachCAM-Gerlach left a comment
edited
Loading

Choose a reason for hiding this comment

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

This is great; thanks@hugovk and@encukou ! I did have some comments; see below.

If adding back legitimate warnings for missing documentation results in the 3.12 What's New no longer being fully clean, you could addlibrary/sqlite3.rst in its place, which Erlend and I have made sure should be clean (documenting what was missing, etc).

hugovkand others added2 commitsMarch 9, 2023 22:34
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@merwok
Copy link
Member

Python repos don’t use force pushes

hugovk reacted with thumbs up emoji

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commentedMar 12, 2023
edited
Loading

In general, PR authors are discouraged from force-pushing once the PR is submitted to avoid making things more difficult for reviewers, but it's not an absolute blind ban—there's still some room for careful judgement, in specific situations where doing so does not conflict with, or in fact supports, the actual reasons for which we have that guideline, i.e. maximizing reviewability.

In particular, when we originally discussed this, it was agreed that it's fine to force-push before a draft PR is marked for review, as it can be a very helpful tool to organize a potentially very noisy initial commit history into an atomic, clearly-explained series of changes, which can greatly aid reviewability.

Likewise, it can potentially make sense and actually aid rather than hurt reviewability to amend a single commit immediately after pushing it to fix a small obvious typo introduced in that commit—which was the case here, with the fixup pushed less than 60 seconds after the original. There's little to no chance any reviewer would have seen, much less review/comment on the commit that quickly, thus there's minimal practical negative impact there. By contrast, it reduces unnecessary noise in the commit history and preserves a single atomic commit for reviewers to look at, without back-and-forth trivial fixups complicating things (which certainly make my life harder as a reviewer). And of course, you can still easily see both commits and compare them with one click if desired in the PR's full conversation history.

hugovk and serhiy-storchaka reacted with thumbs up emoji

@hugovk
Copy link
MemberAuthor

Is there anything else needed here?

Copy link
Member

@ezio-melottiezio-melotti left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Member

@CAM-GerlachCAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM from me (minus a nit); thanks@hugovk

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@hugovk
Copy link
MemberAuthor

Fixed and all green now!

  • PyErr_DisplayException was documented, but the markup was broken
  • PyErr_Display isn't documented, but it's deprecated, so let's skip the reference via!
  • asyncio.iscoroutine wasn't yet documented, but is now
CAM-Gerlach reacted with hooray emoji

@miss-islington
Copy link
Contributor

Status check is done, and it's a success ✅.

@hugovk
Copy link
MemberAuthor

Thanks for checking Miss Islington! Shouldn't you automerge too?

AlexWaygood reacted with laugh emoji

@hugovkhugovk merged commit6a1c49a intopython:mainMar 24, 2023
@hugovkhugovk deleted the docs-nit-picky branchMarch 24, 2023 11:23
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commentedMar 24, 2023
edited
Loading

Maybe its time to just switch to regular GitHub automerge? Is there a reason to still use the old system?

@hugovk
Copy link
MemberAuthor

hugovk commentedMar 24, 2023
edited
Loading

Yeah, it's time! Please could you create ahttps://github.com/python/core-workflow/ issue? We'll need to coordinate deleting somehttps://github.com/python/miss-islington code.


TODO:

  • Once merged, backportexceptions (new in 3.12), weakref, asyncio-tasks and gzip docs to 3.11/3.10.

Backport PRs:

@CAM-Gerlach
Copy link
Member

Sure, I openedpython/core-workflow#498 .

We'll need to coordinate deleting somepython/miss-islington code.

The code might still be needed to support automerge-by-default for miss-islington PRs, if GItHub's API doesn't have a way for her to enable it when she makes them. But I discussed that further on the issue.

hugovk reacted with thumbs up emoji

@brandtbucher
Copy link
Member

I think this check nowbreaks when encountering NEWS entries in theCore and Builtins directory:

Error: One of your files includes a space. Consider using a different output format or removing spaces from your filenames. Please submit an issue on this action's GitHub repo.Error: One of your files includes a space. Consider using a different output format or removing spaces from your filenames.All: Doc/includes/typestruct.h Include/cpython/object.h Include/internal/pycore_code.h Include/internal/pycore_opcode.h Lib/importlib/_bootstrap_external.py Lib/opcode.py Lib/test/test_dis.py Lib/test/test_sys.py Misc/NEWS.d/next/Core and Builtins/2023-03-23-22-31-15.gh-issue-93533.RyrB_g.rst Objects/typeobject.c Programs/test_frozenmain.h Python/bytecodes.c Python/generated_cases.c.h Python/opcode_metadata.h Python/specialize.cAdded: Misc/NEWS.d/next/Core and Builtins/2023-03-23-22-31-15.gh-issue-93533.RyrB_g.rstModified: Doc/includes/typestruct.h Include/cpython/object.h Include/internal/pycore_code.h Include/internal/pycore_opcode.h Lib/importlib/_bootstrap_external.py Lib/opcode.py Lib/test/test_dis.py Lib/test/test_sys.py Objects/typeobject.c Programs/test_frozenmain.h Python/bytecodes.c Python/generated_cases.c.h Python/opcode_metadata.h Python/specialize.cRemoved: Renamed: Added or modified: Doc/includes/typestruct.h Include/cpython/object.h Include/internal/pycore_code.h Include/internal/pycore_opcode.h Lib/importlib/_bootstrap_external.py Lib/opcode.py Lib/test/test_dis.py Lib/test/test_sys.py Misc/NEWS.d/next/Core and Builtins/2023-03-23-22-31-15.gh-issue-93533.RyrB_g.rst Objects/typeobject.c Programs/test_frozenmain.h Python/bytecodes.c Python/generated_cases.c.h Python/opcode_metadata.h Python/specialize.cAdded, modified or renamed: Doc/includes/typestruct.h Include/cpython/object.h Include/internal/pycore_code.h Include/internal/pycore_opcode.h Lib/importlib/_bootstrap_external.py Lib/opcode.py Lib/test/test_dis.py Lib/test/test_sys.py Misc/NEWS.d/next/Core and Builtins/2023-03-23-22-31-15.gh-issue-93533.RyrB_g.rst Objects/typeobject.c Programs/test_frozenmain.h Python/bytecodes.c Python/generated_cases.c.h Python/opcode_metadata.h Python/specialize.c

@hugovk
Copy link
MemberAuthor

Please see PR#103019.

@encukou
Copy link
Member

PyErr_Display isn't documented, but it's deprecated, so let's skip the reference via!

I don't think this is good general policy. Someone encountering a deprecated function in code they inherited should be able to find out what it does and what to use instead.

Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull requestMar 27, 2023
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>Co-authored-by: Petr Viktorin <encukou@gmail.com>
warsaw pushed a commit to warsaw/cpython that referenced this pull requestApr 11, 2023
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>Co-authored-by: Petr Viktorin <encukou@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@merwokmerwokmerwok left review comments

@CAM-GerlachCAM-GerlachCAM-Gerlach approved these changes

@AlexWaygoodAlexWaygoodAlexWaygood left review comments

@ezio-melottiezio-melottiezio-melotti approved these changes

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

9 participants
@hugovk@merwok@CAM-Gerlach@miss-islington@brandtbucher@encukou@ezio-melotti@AlexWaygood@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp