-
Notifications
You must be signed in to change notification settings - Fork 24
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
Added type annotations. #83
Added type annotations. #83
Conversation
I forgot to mention, this pull request is intended to resolve issue #69. |
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.
Looks mostly good! Few changes but thank you for the submission!
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've looked this over, I have a few more suggested changes. But looking pretty close to me.
Thanks for working on this @UnicycleDumpTruck!
Co-authored-by: tekktrik <89490472+tekktrik@users.noreply.github.com>
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.
Looks good to me. Thank you @UnicycleDumpTruck!
Updating https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI to 4.1.0 from 4.0.2: > Merge pull request adafruit/Adafruit_CircuitPython_ESP32SPI#159 from Neradoc/staticIP-hostname-and-dns Updating https://github.com/adafruit/Adafruit_CircuitPython_PCF8563 to 1.0.2 from 1.0.1: > Merge pull request adafruit/Adafruit_CircuitPython_PCF8563#4 from tekktrik/doc/add-typing > Fixed readthedocs build > Consolidate Documentation sections of README Updating https://github.com/adafruit/Adafruit_CircuitPython_RGB_Display to 3.10.12 from 3.10.11: > Merge pull request adafruit/Adafruit_CircuitPython_RGB_Display#101 from makermelissa/master > Fixed readthedocs build > Consolidate Documentation sections of README Updating https://github.com/adafruit/Adafruit_CircuitPython_Debouncer to 2.0.0 from 1.6.0: > Merge pull request adafruit/Adafruit_CircuitPython_Debouncer#38 from Neradoc/better-button-class Updating https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer to 0.1.2 from 0.1.1: > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#1 from askpatrickw/fix-install-commmand Updating https://github.com/adafruit/Adafruit_CircuitPython_MacroPad to 2.1.0 from 2.0.6: > Merge pull request adafruit/Adafruit_CircuitPython_MacroPad#39 from Neradoc/setup-layout-on-init Updating https://github.com/adafruit/Adafruit_CircuitPython_MagTag to 2.1.9 from 2.1.8: > Merge pull request adafruit/Adafruit_CircuitPython_MagTag#83 from UnicycleDumpTruck/add-type-annotations Updating https://github.com/adafruit/Adafruit_CircuitPython_Slideshow to 1.7.4 from 1.7.3: > Merge pull request adafruit/Adafruit_CircuitPython_Slideshow#42 from tekktrik/doc/add-typing > Fixed readthedocs build > Consolidate Documentation sections of README
The return type of magtag.fetch() in magtag.py could probably be more specific than Any, but I didn't feel qualified to make that judgement.
I used Union[float, int] for the frequency and duration arguments of play_tone in peripherals.py. This was partly an accommodation for beginners and for the existing example in the module. Also, the underlying simpleio.tone() function (though not type annotated) documents frequency as float, but casts frequency to int. Simpleio.tone() passes duration to time.sleep, which can take a float or an int.
Thank you for your time, and for the opportunity to participate in the community!