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

Merged
merged 28 commits into from
Jun 25, 2025

Conversation

herobank110
Copy link
Contributor

@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
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
Contributor 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)

Comment on lines +777 to +780
# 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it could introduce breaking changes to remove or alias the imports, eg if users wrote from pytestqt.qtbot import TimeoutError

Another option to reduce confusion in the member variables: eg SignalEmittedError = wait_signal.SignalEmittedError

@herobank110
Copy link
Contributor Author

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)

Thanks both for reviewing. Changed to use the Type Hinting Generics in Standard Collections. Saving the rest for later if its OK :)

@The-Compiler The-Compiler force-pushed the type_annotation_test branch from 37194d2 to 5e4dbd5 Compare June 25, 2025 09:14
@The-Compiler
Copy link
Member

Looking great, thanks again!

@The-Compiler The-Compiler merged commit 8b2c0cb into pytest-dev:master Jun 25, 2025
97 of 98 checks passed
@The-Compiler
Copy link
Member

Ugh, meant to squash merge but clicked too fast.... sorry @nicoddemus 😅

@nicoddemus
Copy link
Member

No worries, it happens. 👍

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