-
Notifications
You must be signed in to change notification settings - Fork 16
Missing type annotations, refs #50 #85
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
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 for working on this @cguardia.
I have a few suggested changes and some questions that I think are worth considering with the wider team. I'd also like to give this a try on a device since there are some changes that go beyond just the type annotations albeit relatively minor, still worth double checking the behavior.
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.
After discussion during the meeting I tested out some alternative approaches that allow us to resolve the mypy errors without needing to have the assert lines. I'll push those tweaks to this branch
@FoamyGuy I'm OK with using the alternative approach to remove assert statements. I made some requested changes, but not these, because you indicated you are already on it. Let me know what else I can do to advance this PR. Thanks, |
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.
The latest version looks good to me. Thanks again for taking this on @cguardia!
I pushed one commit to tweak some of the Display asserts to check for not None instead of Display in order to avoid needing the Display import at runtime. I confirmed that change is acceptable to mypy compared to the Display isinstance check.
I tested all of the micro-controller examples successfully on a PyPortal Titano.
Updating https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_Layout to 1.19.15 from 1.19.14: > Merge pull request adafruit/Adafruit_CircuitPython_DisplayIO_Layout#85 from cguardia/displayio_type_annotations Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
This PR adds type declarations and mypy compliance to this package, as discussed on issue #50. The files were tested with mypy running in strict mode, to make sure no type declaration issues remained. In some cases, it was necessary to modify the code to insure compliance.
Sorry for the long commit. I grabbed the first free issue at the sprint and turned out to be a larger undertaking than expected.