Adding annotations #37
Conversation
There was a problem hiding this comment.
Thanks for working on this @perkin-borowa!
I have a few suggested changes, and some that I am less sure about but will do some testing to verify. Also one about re-using an Alias from our typing library that I'm interested in input from some other team members on.
| @@ -31,7 +37,7 @@ | |||
| __repo__ = "https://github.com/adafruit/Adafruit_CircuitPython_Display_Button.git" | |||
|
|
|||
|
|
|||
| def _check_color(color): | |||
| def _check_color(color: Optional[Union[int, tuple[int, int, int]]]) -> Optional[int]: | |||
There was a problem hiding this comment.
We have an Alias that covers these types of Color arguments. But it's in a file that associates it with leds: https://github.com/adafruit/Adafruit_CircuitPython_Typing/blob/bd358db434d2e2c155c897d7f3c9674b04a3b67f/circuitpython_typing/led.py#L18
@tekktrik do you think it's worth making a slightly more generic Alias for Color inside of circuitpython_typing that can be used in other libraries for hex / tuple colors? Many of our libraries expect both in the context of displayio as well as LEDs.
Or alternatively is it problematic to use the typing from inside of led.py here even though this context isn't dealing with LEDs?
There was a problem hiding this comment.
Hi, let me know when you decide on the way forward with Color implementation and I am happy to import it from appropriate module : )
There was a problem hiding this comment.
I've put a note to discuss this in the weeds for the upcoming weekly meeting.
There was a problem hiding this comment.
I think having multiple aliases is a good idea, sorry for the late response!
There was a problem hiding this comment.
We could always change up the way that colors are defined, potentially moving them out of led.py and into a new file like color.py.
| style: int = RECT, | ||
| fill_color: Optional[int] = 0xFFFFFF, | ||
| outline_color: Optional[int] = 0x0, | ||
| label: Optional[Label] = None, |
There was a problem hiding this comment.
label is expecting a string type. It will eventually put that string into a Label object, but when passed in here it should be a string I believe.
There was a problem hiding this comment.
Hi, so here is where I was struggling during Sprint. When label is not Label object, both property and property setter for it throw errors, as we utilize Label object's arguments (.text, .bounding_box etc.) on variable we hinted as string.
Is there any way to work-around this issue, so mypy does not throw errors, when checking?
Thanks!
There was a problem hiding this comment.
@perkin-borowa I think the type hinter may be confused by this line in the init function:
self._label = label
This is setting the argument label onto self._label which is technically mixing types. The rest of the code is written to assume that self._label will be a Label object.
It never caused any other trouble because the last line of the init function ends up superceding this one that is erroneously setting self._label to the string argument.
self.label = label
That line calls the setter which is expecting the text to get passed into it.
Try removing
self._label = label
And see if mypy still reports any errors about Label object vs. string for the argument.
| @property | ||
| def label(self): | ||
| def label(self) -> Optional[Union[Any, str]]: |
There was a problem hiding this comment.
I'm not sure we need Any here for the return type. Are there cases where the return will be something other than a string or None?
Adding type-hints for the module. Added asserts needed for type checking, as well as moved the init function to the beginning of the class for additional compliance with mypy.