Skip to content

Type annotation infrastructure and initial typing for qtbot.py #605

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

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

herobank110
Copy link

@herobank110 herobank110 commented May 22, 2025

Initial version of adding type annotations to the library.

  • infrastructure of mypy in precommit (ci and local type checking)

  • Typing focused around qtbot.py to start with

  • Note: qt objects are typed with aliases refering to Any - this may be upgraded in future to use correct types.

Fixes #417

@herobank110 herobank110 marked this pull request as draft May 22, 2025 22:39
@herobank110 herobank110 mentioned this pull request May 22, 2025
@herobank110 herobank110 changed the title Type annotation test Type annotation infrastructure and initial typing for qtbot.py Jun 8, 2025
@herobank110 herobank110 marked this pull request as ready for review June 8, 2025 20:14
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks a lot @herobank110!

Left some comments, please take a look!

@nicoddemus nicoddemus requested a review from The-Compiler June 10, 2025 13:19
nicoddemus
nicoddemus previously approved these changes Jun 13, 2025
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks a lot @herobank110, this looks great!

nicoddemus
nicoddemus previously approved these changes Jun 13, 2025
@nicoddemus
Copy link
Member

nicoddemus commented Jun 13, 2025

Thanks @herobank110!

Leaving it open for a few days to give @The-Compiler a chance to review too. 👍

@herobank110
Copy link
Author

Thanks @herobank110!

Leaving it open for a few days to give @The-Compiler a chance to review too. 👍

Keen to continue contributing with the typing and other stuff you may have

Copy link
Member

@The-Compiler The-Compiler 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, I'm pretty excited for this! ✨

I added a couple of comments for things I noticed, mostly they're minor things and shouldn't block anything though. A todo-list for follow-ups if we don't do them as part of this PR:

  • Use Type Hinting Generics in Standard Collections (list, dict, type over typing.List etc.)
  • Use from __future__ import annotations
    • Use PEP 604: New Type Union Operator, i.e. TheType | None instead of Optional[TheType]
    • Use "bare" annotations for forward declarations instead of stringified ones
    • Configure ruff to enforce from __future__ import annotations on every file for consistency
  • Remove redundant type annotations from Sphinx docstrings (and possibly configure Sphinx to pick up the annotations, not sure what's needed for that nowadays)

@@ -0,0 +1,9 @@
[mypy]
exclude = ^docs/
no_implicit_optional = True
Copy link
Member

Choose a reason for hiding this comment

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

This is already the default since mypy 0.980 (~September 2022).

Comment on lines +11 to +12
CapturedException = Tuple[Type[BaseException], BaseException, TracebackType]
CapturedExceptions = List[CapturedException]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CapturedException = Tuple[Type[BaseException], BaseException, TracebackType]
CapturedExceptions = List[CapturedException]
CapturedException = tuple[type[BaseException], BaseException, TracebackType]
CapturedExceptions = list[CapturedException]

(possible since Python 3.9 which is now the earliest version we support, and avoids having to import all that stuff from typing).

@@ -358,13 +403,13 @@ def waitSignal(self, signal, *, timeout=5000, raising=None, check_params_cb=None

def waitSignals(
self,
signals,
signals: List[SignalInstance],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
signals: List[SignalInstance],
signals: list[SignalInstance],

):
timeout: int = 5000,
raising: Optional[bool] = None,
check_params_cbs: Optional[List[CheckParamsCb]] = None,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
check_params_cbs: Optional[List[CheckParamsCb]] = None,
check_params_cbs: Optional[list[CheckParamsCb]] = None,

Comment on lines +777 to +781
# provide easy access to exceptions to qtbot fixtures
SignalEmittedError = SignalEmittedError
TimeoutError = TimeoutError
ScreenshotError = ScreenshotError
CallbackCalledTwiceError = CallbackCalledTwiceError
Copy link
Member

Choose a reason for hiding this comment

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

Those seem somewhat confusing - I wonder if we should from ... import SignalEmittedError as SignalEmittedError_ or something to make clear what happens here?


def _add_widget(item, widget, *, before_close_func=None):
def _add_widget(
item: Any,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe

Suggested change
item: Any,
item: pytest.Item,

and add a type ignore about us monkeypatching stuff on it? Long-term, we should be using Stash instead anyways.

Same also for the other utility methods taking item below.

@@ -769,7 +824,9 @@ class _WaitWidgetContextManager:
Context manager implementation used by ``waitActive`` and ``waitExposed`` methods.
"""

def __init__(self, method_name, adjective_name, widget, timeout):
def __init__(
self, method_name: str, adjective_name: str, widget: QWidget, timeout: int
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self, method_name: str, adjective_name: str, widget: QWidget, timeout: int
self, method_name: str, adjective_name: Literal["activated", "exposed"], widget: QWidget, timeout: int

maybe, based on the docstring?

def __exit__(self, exc_type, exc_val, exc_tb):
def __exit__(
self,
exc_type: Optional[Type[BaseException]],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
exc_type: Optional[Type[BaseException]],
exc_type: Optional[type[BaseException]],

@@ -1,10 +1,12 @@
import functools
import dataclasses
from typing import Any
from typing import Any, Callable
Copy link
Member

Choose a reason for hiding this comment

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

typing.Callable is deprecated, this should be using collections.abc.Callable instead.

import weakref
import warnings
from typing import (
TYPE_CHECKING,
Callable,
Copy link
Member

Choose a reason for hiding this comment

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

typing.Callable is deprecated, this should be using collections.abc.Callable instead.

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

Successfully merging this pull request may close these issues.

Type annotations
3 participants