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

refactor: add type annotations and improve type safety #1205

Merged
merged 8 commits into from
Feb 11, 2025

Conversation

KarimAziev
Copy link
Contributor

@KarimAziev KarimAziev commented Feb 6, 2025

This PR introduces significant improvements to type hinting as part of resolving issue #1192.

  • The most important change was adding overloads to dispatch_functions, ensuring that it now correctly propagates return types.
  • Refactored the Job class into a generic type.
  • Added type annotations to core methods such as capture_array, capture_image, and create_video_configuration.
  • Enhanced runtime safety by adding more assert statements.
  • Fixed inconsistencies in the return types of private helper functions.
  • Improved the pyright configuration: disabled the reportUnknownMemberType warning, resulting in fewer but more meaningful warnings being displayed.

While the PR mainly focuses on typing improvements with no logical changes, there's a minor fix in Job.cancel():

Before:

self._future.set_exception(CancelledError)

Now:

self._future.set_exception(CancelledError())

This change ensures that set_exception is always given an instance of CancelledError, aligning with concurrent.futures.Future's expectations. The previous version would have raised a TypeError: exceptions must be derived from BaseException when raised if it were ever encountered at runtime. This fix does not alter any behavior but ensures correctness.

P.S. I didn't plan for this to be so extensive, but... it turned into a rabbit hole.

@davidplowman
Copy link
Collaborator

Thanks for this PR. Looks like I need to install a newer version of typing-extensions than the Bookworm default. I'll do that and then re-open this so that it runs again.

@davidplowman
Copy link
Collaborator

That looks good, there's just one snag I've run into related to the line

from typing_extensions import Buffer

Because we ship Picamera2 installed in our regular OS images, it means that I can't use a version of typing_extensions that isn't in our Bookworm apt repo. The version currently there is 4.4.0. But for this line to work I think I need at least 4.8.0.

So the solutions are probably:

  • Either update the Bookworm apt version to 4.8.0 (or newer)
  • Or remove this line and the usage of the Buffer type later in the file.

I'll see whether it looks possible to update the Bookworm repo (does 4.8.0 sound OK to you?) and then post back.

Thanks again!

@KarimAziev
Copy link
Contributor Author

Thanks for the feedback! I've removed it for now, so an updated version of typing-extensions is no longer needed.

@davidplowman davidplowman merged commit 1898df0 into raspberrypi:next Feb 11, 2025
5 checks passed
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.

2 participants