Skip to content

Add type hints #37

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 5, 2023
Merged

Add type hints #37

merged 2 commits into from
Jun 5, 2023

Conversation

OptionalLion411
Copy link
Contributor

I added type hints as mentioned by @FoamyGuy in #31.

The only missing parts are the protected members _servo() and _motor().
I'm not sure if these should be added, but they could be solved by adding TypeVars at the toplevel

try:
    from typing import Tuple, Type, TypeVar, Union
    S = TypeVar("S")
    M = TypeVar("M")
except ImportError:
    pass

and changing the function signatures as follows in a subsequent commit:

def _servo(self, terminal: int, servo_class: Type[S]) -> S:

...

def _motor(self, terminals: Tuple[int, ...], motor_class: Type[M]) -> M:

Thank for your review and with regards!

FoamyGuy
FoamyGuy approved these changes Jun 5, 2023
View reviewed changes
Copy link
Contributor

@FoamyGuy FoamyGuy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks good to me. thank you @OptionalLion411.

I think the internal _servo and _motor are okay as-is for now. the TypeVar("S") thing doesn't seem to add any additional information to the type if I am understanding correctly. Perhaps at some point we could add aliases inside of circuitpython_typing repo for the valid types of servos and motors that can be returned.

I tested the new version successfully with a Crickit Featherwing using the neopixels example from this repo.

Neopixels looked like the only portion of the change that could possible be functionally different when run on the microcontrollers due to the import order change (though it really shouldn't make a difference). I confirmed that the neopixels connected to the terminal do work as expected with this version.

I'm not sure why the original version had the import occurring "out of order" down inside the property. Maybe something to do with the way the Featherwing Crickit and CPX Crickit differ with regards to what the neopixel terminals are connected to

@FoamyGuy FoamyGuy merged commit a1f6652 into adafruit:main Jun 5, 2023
@FoamyGuy FoamyGuy mentioned this pull request Jun 5, 2023
Closed
7 tasks
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jun 6, 2023
@adafruit-adabot
Automated update by Adabot (adafruit/adabot@b417f43)
d5b82fd 
Updating https://github.com/adafruit/Adafruit_CircuitPython_BME680 to 3.5.0 from 3.4.13:
  > Merge pull request adafruit/Adafruit_CircuitPython_BME680#61 from garberw/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_Crickit to 2.3.15 from 2.3.14:
  > Merge pull request adafruit/Adafruit_CircuitPython_Crickit#37 from OptionalLion411/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_MatrixKeypad to 1.2.14 from 1.2.13:
  > Merge pull request adafruit/Adafruit_CircuitPython_MatrixKeypad#16 from gtbcoding/isssue/13-type-annotation

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FoamyGuy FoamyGuy FoamyGuy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@OptionalLion411 @FoamyGuy