-
Notifications
You must be signed in to change notification settings - Fork 33
Add missing type annotations #63
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
Add missing type annotations #63
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. I've noted a few places where there are function arguments that we can add types for.
cc9a8fc
to
e4631db
Compare
Already add missing types |
@kattni problem with docs and imports? |
Thanks @pedrovs16! I requested to be tagged so I can verify the documentation still looks right on RTD with the imports before the initial docstring. I'll look into it following the merge and release. |
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 following up.
I've noted a few more changes after having another look over this.
The rest is looking good to me beyond the things noted.
adafruit_irremote.py
Outdated
self, input_pulses, max_pulse=10000, pulse_window=0.10 | ||
): # pylint: disable=no-self-use | ||
def _read_pulses_non_blocking( # pylint: disable=no-self-use | ||
self, input_pulses: List, max_pulse: int = 10000, pulse_window: int = 0.10 |
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.
pulse_window
looks like float
I think instead of int
adafruit_irremote.py
Outdated
max_pulse: int = 10000, | ||
blocking: bool = True, | ||
pulse_window: bool = 0.10, | ||
blocking_delay: bool = 0.10, |
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.
both pulse_window
and blocking_delay
look like they can be float
instead of bool
adafruit_irremote.py
Outdated
@@ -341,14 +351,24 @@ class GenericTransmit: | |||
:param bool debug: Enable debug output, default False | |||
""" | |||
|
|||
def __init__(self, header, one, zero, trail, *, debug=False): | |||
def __init__( | |||
self, header: int, one: int, zero: int, trail: int, *, debug=False |
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.
debug
can be typed as bool
adafruit_irremote.py
Outdated
*, | ||
repeat: int = 0, | ||
delay: int = 0, | ||
nbits: int = None, |
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.
for nbits
since the default value is None
I think it can be Optional[int]
7fa7d72
to
2f4f7e7
Compare
2f4f7e7
to
42a046e
Compare
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.
This looks good to me now. Thanks again for working on it @pedrovs16!
I made one commit with a tweak to us float instead of int for delay. It was typed as float in the sphinx docstring already and I confirmed on a device that passing floats is acceptable.
Updating https://github.com/adafruit/Adafruit_CircuitPython_IRRemote to 4.1.15 from 4.1.14: > Merge pull request adafruit/Adafruit_CircuitPython_IRRemote#63 from pedrovs16/add-missing-type-annotations > Update pre-commit hooks Updating https://github.com/adafruit/Adafruit_CircuitPython_Logging to 5.2.3 from 5.2.1: > Merge pull request adafruit/Adafruit_CircuitPython_Logging#50 from FoamyGuy/jquery_fix > Merge pull request adafruit/Adafruit_CircuitPython_Logging#49 from tyeth/patch-1 > Update pre-commit hooks > Add upload url to release action Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
Fixing to issue #50