search

adafruit / Adafruit_CircuitPython_DisplayIO_Layout

A Circuitpython helper library for display element layout using displayio.
MIT License
10 stars 14 forks source link

Missing Type Annotations #50

Closed FoamyGuy closed 1 year ago

FoamyGuy commented 3 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:

cguardia commented 1 year ago

Picking this one up for the PyCon 2023 sprint.

cguardia commented 1 year ago

@FoamyGuy Hi, working on this, but I'm a bit stumped. in switch_round.py , selected and contains methods are calling the superclass, with a touchpoint argument. According to the docs in the comments, touchpoint should be of type Tuple[float, float], but the calls here include a third parameter, which is not expected by the superclass:

https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_Layout/blob/main/adafruit_displayio_layout/widgets/switch_round.py#L810

Why the extra parameter? Am I missing something? Thanks for helping out.

jposada202020 commented 1 year ago

@cguardia Hiya, the extra parameter is the pressure that is needed for the touchscreen.

https://github.com/adafruit/Adafruit_CircuitPython_Touchscreen/blob/d392b3135ced990cda052536b8451a37238fb540/adafruit_touchscreen.py#L128

cguardia commented 1 year ago

@jposada202020 Thanks a lot. So I guess that means I should change all doc strings that refer to this as just an x,y tuple, right?

jposada202020 commented 1 year ago

@cguardia I think is a good idea. We could document, afterwards, if the pressure parameter is exposed to the user. A question, you mentioned a tuple of floats, I think they are ints. But I might be wrong.

FoamyGuy commented 1 year ago

touchpoints should indeed contain int values. And agree with the above, we can update the documentation to mention the optional pressure argument (I don't think all screen types have this capability so some will not have that 3rd value)

Perhaps: Tuple[int, int, Optional[int]]

cguardia commented 1 year ago

@FoamyGuy Thanks for the guidance. One more question: calls that use touch_point as an argument usually get List[int] in the call, rather than the tuple. Should we allow lists as well on the type description, or change calls that pass in lists to pass in tuples instead?

FoamyGuy commented 1 year ago

@cguardia I've had a poke around and I think we want to allow tuples or lists, at least for now. the Touchscreen library uses tuples, but this other capacitive touch screen library says it returns lists: https://docs.circuitpython.org/projects/focaltouch/en/latest/api.html#adafruit_focaltouch.Adafruit_FocalTouch.touches