Skip to content
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-104050: Run mypy on clinic.py in CI #104421

Merged
merged 16 commits into from
May 15, 2023

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented May 12, 2023

Add a basic mypy check to clinic.py in CI (and add some initial type annotations, so that the mypy check passes).

The check will run:

  • On pushes to main
  • On pull requests that touch files in the Tools/clinic directory, and pull requests that touch the workflow file itself
  • When contributors manually request a workflow on their fork, via workflow_dispatch.

When it does run, mypy will only check files inside Tools/clinic/. All other files will be excluded from the mypy check in CI.

The mypy configuration is also deliberately lax, to begin with: only a few of the stricter settings are enabled. This will allow us to make use of "gradual typing": mypy will only check functions inside Tools/clinic/ that we've added annotations to. It will assume all other functions are to be ignored by mypy, for now; we will therefore be able to gradually increase the level of type annotations inside this directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

I usually put my mypy config in a pyproject.toml file these days, but a mypy.ini file feels like it makes more "sense" unless we're planning on adding other linters/formatters to check clinic.py in the future

.github/workflows/mypy-clinic.yml Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Show resolved Hide resolved
Tools/clinic/clinic.py Show resolved Hide resolved
Tools/clinic/cpp.py Outdated Show resolved Hide resolved
Tools/clinic/cpp.py Outdated Show resolved Hide resolved
Tools/clinic/cpp.py Outdated Show resolved Hide resolved
.github/workflows/mypy-clinic.yml Outdated Show resolved Hide resolved
.github/workflows/mypy-clinic.yml Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Show resolved Hide resolved
.github/workflows/mypy-clinic.yml Outdated Show resolved Hide resolved
AlexWaygood and others added 3 commits May 12, 2023 14:18
.github/workflows/mypy.yml Outdated Show resolved Hide resolved
.github/workflows/mypy.yml Outdated Show resolved Hide resolved
Tools/clinic/requirements-dev.txt Outdated Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <[email protected]>
@@ -0,0 +1,2 @@
# Requirements file for external linters and checks we run on Tools/clinic/ in CI
mypy==1.2.0
Copy link
Member Author

Choose a reason for hiding this comment

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

We're deliberately pinning to an older version here, in order to test that dependabot updates are working on this file, as per @hugovk's suggestion in #104421 (comment)

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

CI changes look good 👍

Tools/clinic/cpp.py Outdated Show resolved Hide resolved
@AlexWaygood
Copy link
Member Author

AlexWaygood commented May 12, 2023

(The test_zoneinfo failure is obviously unrelated.)

Edit: As is the test_threading failure.

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@AlexWaygood
Copy link
Member Author

LGTM. Thanks!

Very happy to help!

@AlexWaygood AlexWaygood enabled auto-merge (squash) May 15, 2023 08:29
@AlexWaygood AlexWaygood merged commit 9d41f83 into python:main May 15, 2023
@AlexWaygood AlexWaygood deleted the clinic-mypy-check branch May 15, 2023 10:22
carljm added a commit to carljm/cpython that referenced this pull request May 15, 2023
* main: (29 commits)
  pythongh-101819: Fix _io clinic input for unused base class method stubs (python#104418)
  pythongh-101819: Isolate `_io` (python#101948)
  Bump mypy from 1.2.0 to 1.3.0 in /Tools/clinic (python#104501)
  pythongh-104494: Update certain Tkinter pack/place tests for Tk 8.7 errors (python#104495)
  pythongh-104050: Run mypy on `clinic.py` in CI (python#104421)
  pythongh-104490: Consistently define phony make targets (python#104491)
  pythongh-67056: document that registering/unregistering an atexit func from within an atexit func is undefined (python#104473)
  pythongh-104487: PYTHON_FOR_REGEN must be minimum Python 3.10 (python#104488)
  pythongh-101282: move BOLT config after PGO (pythongh-104493)
  pythongh-104469 Convert _testcapi/float.c to use AC (pythongh-104470)
  pythongh-104456: Fix ref leak in _ctypes.COMError (python#104457)
  pythongh-98539: Make _SSLTransportProtocol.abort() safe to call when closed (python#104474)
  pythongh-104337: Clarify random.gammavariate doc entry  (python#104410)
  Minor improvements to typing docs (python#104465)
  pythongh-87092: avoid gcc warning on uninitialized struct field in assemble.c (python#104460)
  pythonGH-71383: IDLE - Document testing subsets of modules (python#104463)
  pythongh-104454: Fix refleak in AttributeError_reduce (python#104455)
  pythongh-75710: IDLE - add docstrings and comments to editor module (python#104446)
  pythongh-91896: Revert some very noisy DeprecationWarnings for `ByteString` (python#104424)
  Add a mention of PYTHONBREAKPOINT to breakpoint() docs (python#104430)
  ...
carljm added a commit to carljm/cpython that referenced this pull request May 15, 2023
* main: (204 commits)
  pythongh-101819: Fix _io clinic input for unused base class method stubs (python#104418)
  pythongh-101819: Isolate `_io` (python#101948)
  Bump mypy from 1.2.0 to 1.3.0 in /Tools/clinic (python#104501)
  pythongh-104494: Update certain Tkinter pack/place tests for Tk 8.7 errors (python#104495)
  pythongh-104050: Run mypy on `clinic.py` in CI (python#104421)
  pythongh-104490: Consistently define phony make targets (python#104491)
  pythongh-67056: document that registering/unregistering an atexit func from within an atexit func is undefined (python#104473)
  pythongh-104487: PYTHON_FOR_REGEN must be minimum Python 3.10 (python#104488)
  pythongh-101282: move BOLT config after PGO (pythongh-104493)
  pythongh-104469 Convert _testcapi/float.c to use AC (pythongh-104470)
  pythongh-104456: Fix ref leak in _ctypes.COMError (python#104457)
  pythongh-98539: Make _SSLTransportProtocol.abort() safe to call when closed (python#104474)
  pythongh-104337: Clarify random.gammavariate doc entry  (python#104410)
  Minor improvements to typing docs (python#104465)
  pythongh-87092: avoid gcc warning on uninitialized struct field in assemble.c (python#104460)
  pythonGH-71383: IDLE - Document testing subsets of modules (python#104463)
  pythongh-104454: Fix refleak in AttributeError_reduce (python#104455)
  pythongh-75710: IDLE - add docstrings and comments to editor module (python#104446)
  pythongh-91896: Revert some very noisy DeprecationWarnings for `ByteString` (python#104424)
  Add a mention of PYTHONBREAKPOINT to breakpoint() docs (python#104430)
  ...
@@ -2558,15 +2570,15 @@ class CConverter(metaclass=CConverterAutoRegister):
"""

# The C name to use for this variable.
name = None
name: str | None = None
Copy link
Contributor

@erlend-aasland erlend-aasland May 16, 2023

Choose a reason for hiding this comment

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

So, these str | None are now starting to cause frustrations, as I gradually add typing to other parts of clinic.py. Since there are no guards anywhere, I obviously get errors and warnings about str plus None, etc. We could change the defaults to the empty string, but that can also create a lot of havoc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants