Skip to content

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

Merged
merged 4 commits into from
Oct 3, 2022
Merged

Add type annotations #86

merged 4 commits into from
Oct 3, 2022

Conversation

tekktrik
Copy link
Member

Resolves #68

@tekktrik tekktrik requested a review from a team August 30, 2022 15:56
Copy link
Contributor

@FoamyGuy FoamyGuy left a 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
@@ -91,49 +99,49 @@ def _parse_degrees(nmea_data):
return degrees + minutes # return parsed string in the format dddmmmmmm
Copy link
Contributor

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.

Copy link
Member Author

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!

# This function loses precision with float32
x = data[index] / 1000000
if data[index + 1].lower() == neg:
x *= -1.0
return x


def _read_int_degrees(data, index, neg):
def _read_int_degrees(data: List[float], index: int, neg: str) -> float:
deg = data[index] // 1000000
minutes = data[index] % 1000000 / 10000
if data[index + 1].lower() == neg:
deg *= -1
return (deg, minutes)
Copy link
Contributor

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

Copy link
Member Author

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,
Copy link
Contributor

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?

Copy link
Member Author

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.

@tekktrik tekktrik requested a review from FoamyGuy September 16, 2022 18:58
Copy link
Contributor

@FoamyGuy FoamyGuy left a 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!

@FoamyGuy FoamyGuy merged commit d8fdb0b into adafruit:main Oct 3, 2022
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Oct 4, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Type Annotations
2 participants