search

adafruit / Adafruit_CircuitPython_GPS

GPS parsing module for CircuitPython. Meant to parse NMEA data from serial GPS modules.
MIT License
75 stars 58 forks source link

Missing Type Annotations #68

Closed FoamyGuy closed 1 year ago

FoamyGuy commented 2 years ago

There are missing type annotations for some functions in this library.

The typing module does not exist on CircuitPython devices so the import needs to be wrapped in try/except to catch the error for missing import. There is an example of how that is done here:

try:
    from typing import List, Tuple
except ImportError:
    pass

Once imported the typing annotations for the argument type(s), and return type(s) can be added to the function signature. Here is an example of a function that has had this done already:

def wrap_text_to_pixels(
    string: str, max_width: int, font=None, indent0: str = "", indent1: str = ""
) -> List[str]:

If you are new to Git or Github we have a guide about contributing to our projects here: https://learn.adafruit.com/contribute-to-circuitpython-with-git-and-github

There is also a guide that covers our CI utilities and how to run them locally to ensure they will pass in Github Actions here: https://learn.adafruit.com/creating-and-sharing-a-circuitpython-library/check-your-code In particular the pages: Sharing docs on ReadTheDocs and Check your code with pre-commit contain the tools to install and commands to run locally to run the checks.

If you are attempting to resolve this issue and need help, you can post a comment on this issue and tag both @foamyguy and @kattni or reach out to us on Discord: https://adafru.it/discord in the #circuitpython-dev channel.

The following locations are reported by mypy to be missing type annotations:

jkittner commented 2 years ago

I had a look at this, because I though it would make it much easier to implement the tests in #69 . However it turns out, some major refactoring has to be done to make this type safe. If anyone wants to continue on this, I pushed the changes I already did to this branch: https://github.com/theendlessriver13/Adafruit_CircuitPython_GPS/tree/type-annotations .

The main thing which causes problems, is the data lists having a variable length depending on the type of data. Ideally each sentence type would be a strongly typed named tuple (not sure if this is available on all boards?) for each sentence type or there would be one Data named tuple with all possible paramters most of them being Optional, so one would only have to check if it was None, not if it was a string, integer or float all the time ...

Also many, many checks for is None are missing to make this type safe...

The inherited class also causes some trouble: adafruit_gps.py:700: error: Return type "None" of "write" incompatible with return type "int" in supertype "GPS"

sha016 commented 2 years ago

@theendlessriver13 I checked out a branch and plugged in what you had come up with and got it passing tests.

@FoamyGuy Should we just merge the type annotation changes for now and worry about refactoring/tests in a separate issue?

jkittner commented 2 years ago

Hey, just to make sure you ran mypy against this? Because when you're talking about tests I am thinking about unit tests or maybe pre-commit which should of also course both pass here, but what's interesting here, is mypy and I still get 60 errors for this.

(venv) jkittner@ububox:~/workspace/Adafruit_CircuitPython_GPS$ pytest
============================= test session starts ==============================
platform linux -- Python 3.7.12, pytest-6.2.5, py-1.10.0, pluggy-1.0.0
rootdir: /home/jkittner/workspace/Adafruit_CircuitPython_GPS
collected 48 items                                                             

tests/adafruit_gps_test.py ............................................. [ 93%]
...                                                                      [100%]

============================== 48 passed in 0.14s ==============================
(venv) jkittner@ububox:~/workspace/Adafruit_CircuitPython_GPS$ mypy adafruit_gps.py  | wc -l
60
sha016 commented 2 years ago

@theendlessriver13 I had not ran tests with mypy (just pytest). I ran mypy tests and got 45 errors and see what you mean about refactoring.

FoamyGuy commented 2 years ago

I think in the short term we are working to add the type hits to resolve Function is missing a type annotation error from mypy to start with. It's okay if it would take a larger refactoring to pass 100% of the mypy checks at this time. We aren't adding the step to CI yet, there will be some discussion to figure out how we want it configured before it gets added.

I'm not very familiar with this library or how it operates so I'm a bit unsure about why some of the things have so many different available types.

It looks like a good start to me that @theendlessriver13 has got in that commit.

I think that named tuples can be used for this purpose as proposed but I'm not 100% certain. If someone picks this up and tries it out that way we can test to ensure no issues on the microcontroller side.