-
-
Notifications
You must be signed in to change notification settings - Fork 69
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
Type annotation infrastructure and initial typing for qtbot.py #605
Conversation
There was a problem hiding this 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!
There was a problem hiding this 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!
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 |
There was a problem hiding this 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
overtyping.List
etc.) - Use
from __future__ import annotations
- Use PEP 604: New Type Union Operator, i.e.
TheType | None
instead ofOptional[TheType]
- Use "bare" annotations for forward declarations instead of stringified ones
- Configure ruff to enforce
from __future__ import annotations
on every file for consistency
- Use PEP 604: New Type Union Operator, i.e.
- Remove redundant type annotations from Sphinx docstrings (and possibly configure Sphinx to pick up the annotations, not sure what's needed for that nowadays)
# provide easy access to exceptions to qtbot fixtures | ||
SignalEmittedError = SignalEmittedError | ||
TimeoutError = TimeoutError | ||
ScreenshotError = ScreenshotError | ||
CallbackCalledTwiceError = CallbackCalledTwiceError |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Thanks both for reviewing. Changed to use the Type Hinting Generics in Standard Collections. Saving the rest for later if its OK :) |
for more information, see https://pre-commit.ci
Co-authored-by: Bruno Oliveira <bruno@soliv.dev>
Co-authored-by: Bruno Oliveira <bruno@soliv.dev>
Co-authored-by: Bruno Oliveira <bruno@soliv.dev>
37194d2
to
5e4dbd5
Compare
Looking great, thanks again! |
Ugh, meant to squash merge but clicked too fast.... sorry @nicoddemus 😅 |
No worries, it happens. 👍 |
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