-
Notifications
You must be signed in to change notification settings - Fork 61
Add type annotations #86
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
FoamyGuy
left a comment
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 @tekktrik. Looking good overall, found a few possible tweaks.
adafruit_gps.py
Outdated
| minutes = int(raw[0]) % 100 # the mm. | ||
| minutes += int(f"{raw[1][:4]:0<4}") / 10000 | ||
| minutes = int(minutes / 60 * 1000000) | ||
| return degrees + minutes # return parsed string in the format dddmmmmmm |
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.
Might be worth updating the comment here, it mentions that a parsed string is returned, but it does appear to be an int same as it's annotated now.
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 ended up just removing it since the comments below the function definition say it's "a pure degrees value". I think that's clear between that, the other comments, and the type annotation, but happy to change to improve clarity. Good catch!
| minutes = data[index] % 1000000 / 10000 | ||
| if data[index + 1].lower() == neg: | ||
| deg *= -1 | ||
| return (deg, minutes) |
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 return looks like it would be a tuple to me instead of float
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.
Whoops, thanks for catching!
adafruit_gps.py
Outdated
| *, | ||
| address: int = _GPSI2C_DEFAULT_ADDRESS, | ||
| debug: bool = False, | ||
| timeout: float = 5, |
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.
should timeout allow int or float Or maybe have it's default value changed to 5.0 instead?
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.
It seems only to be used with time.monotonic() so float is a valid input. I'm in favor of changing to 5.0 if that helps makes that clearer.
FoamyGuy
left a comment
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. Thanks @tekktrik!
Updating https://github.com/adafruit/Adafruit_CircuitPython_ESP_ATcontrol to 0.8.0 from 0.7.0: > Merge pull request adafruit/Adafruit_CircuitPython_ESP_ATcontrol#63 from bablokb/fix-retries Updating https://github.com/adafruit/Adafruit_CircuitPython_GPS to 3.10.5 from 3.10.4: > Merge pull request adafruit/Adafruit_CircuitPython_GPS#86 from tekktrik/doc/add-typing Updating https://github.com/adafruit/Adafruit_CircuitPython_INA260 to 1.3.11 from 1.3.10: > Merge pull request adafruit/Adafruit_CircuitPython_INA260#21 from gpongelli/patch-3 Updating https://github.com/adafruit/Adafruit_CircuitPython_LIS331 to 1.0.13 from 1.0.12: > Merge pull request adafruit/Adafruit_CircuitPython_LIS331#6 from tcfranks/main Updating https://github.com/adafruit/Adafruit_CircuitPython_turtle to 2.2.11 from 2.2.10: > Merge pull request adafruit/Adafruit_CircuitPython_turtle#31 from shulltronics/cpython-compatibility
Resolves #68